maria-developers team mailing list archive

[Kristian Nielsen <knielsen@xxxxxxxxxxxxxxx>]

Michael Widenius <monty@xxxxxxxxxxxx> writes:

>>>>>> "noreply" == noreply  <noreply@xxxxxxxxxxxxx> writes:
>
> noreply> === modified file 'mysys/my_getopt.c'
> noreply> --- mysys/my_getopt.c	2009-04-25 10:05:32 +0000
> noreply> +++ mysys/my_getopt.c	2009-05-20 15:34:34 +0000
> noreply> @@ -647,7 +647,7 @@
> noreply>  	return EXIT_OUT_OF_MEMORY;
> noreply>        break;
> noreply>      case GET_ENUM:
> noreply> -      if (((*(int*)result_pos)= find_type(argument, opts->typelib, 2) - 1) < 0)
> noreply> +      if (((*(ulong *)result_pos)= find_type(argument, opts->typelib, 2) - 1) < 0)
> noreply>          return EXIT_ARGUMENT_INVALID;
> noreply>        break;
> noreply>      case GET_SET:
> noreply> @@ -983,7 +983,7 @@
> noreply>      *((int*) variable)= (int) getopt_ll_limit_value((int) value, option, NULL);
> noreply>      break;
> noreply>    case GET_ENUM:
> noreply> -    *((uint*) variable)= (uint) value;
> noreply> +    *((ulong*) variable)= (uint) value;
> noreply>      break;
> noreply>    case GET_UINT:
> noreply>      *((uint*) variable)= (uint) getopt_ull_limit_value((uint) value, option, NULL);
> noreply> @@ -1221,7 +1221,7 @@
> noreply>  	}
> noreply>  	break;
> noreply>        case GET_ENUM:
> noreply> -        printf("%s\n", get_type(optp->typelib, *(uint*) value));
> noreply> +        printf("%s\n", get_type(optp->typelib, *(ulong*) value));
> noreply>  	break;
> noreply>        case GET_STR:
> noreply>        case GET_STR_ALLOC:                    /* fall through */
>
> Where is the enum used that caused this to break ?
> (I did a grep of GET_ENUM in the current tree, but couldn't find anything.
>
> As an ENUM can only contain a small set of values, it would be more
> logical that it should be an uint ant not an ulong.
> (With some 64 bit compilers, uint -> 32 bit while ulong is 64 bit, so
> there is a clear advantage in using uint)
>
> Becasue of this, I am more included to belive the the one that is
> using GET_ENUM as ulong is doing things wrong instead of the above
> code.

The test case that failed was plugin_load.test, which tests the option
--loose-plugin-example-enum-var=e2 of the example plugin.

However, as I understand it the problem here is one that has come up many
times with how the command line / configuration options are handled.

Each option is defined with a generic void * as the value, and that is then
cast to/from the actual pointer type when accessed.

And I believe number option values are supposed to be of type long.

Due to the casting of the pointers, there is no compiler type checking, and it
is common to get this wrong so that the value is accessed sometimes as int and
sometimes as long. Like in this case.

The problem is that the failure is usually only seen on 64-bit big-endian
machines, of which there are not many around these days.

I think the Drizzle people did something with the option parsing to remove the
problem of missing type checking. Probably the better fix would be to do
something similar, but that is well out of the scope of this fix.

Anyway, it is true that enums could use type uint rather than ulong. However,
as I understand it, everywhere else in the code numeric option values are
always type long (I seem to remember I briefly checked the code for similar
mistakes). And consistently using long seems at least slightly less error
prone than using sometimes one and sometimes the other. I do not have any
strong opinions on the matter though.

 - Kristian.