Re: [Xen-devel] [PATCH v2] xen/cmdline: Fix buggy strncmp(s, LITERAL, ss - s) construct

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.01.19 at 18:17, <andrew.cooper3@xxxxxxxxxx> wrote:
> --- a/xen/arch/arm/efi/efi-boot.h
> +++ b/xen/arch/arm/efi/efi-boot.h
> @@ -212,7 +212,7 @@ EFI_STATUS __init fdt_add_uefi_nodes(EFI_SYSTEM_TABLE 
> *sys_table,
>              break;
>  
>          type = fdt_getprop(fdt, node, "device_type", &len);
> -        if ( type && strncmp(type, "memory", len) == 0 )
> +        if ( type && len == 6 && strncmp(type, "memory", 6) == 0 )

So why not memcmp() then, which is generally cheaper?

> @@ -271,6 +297,27 @@ int parse_boolean(const char *name, const char *s, const 
> char *e)
>      return -1;
>  }
>  
> +int cmdline_strcmp(const char *frag, const char *name)

So you've decided to retain the strcmp()-like return type and value,
despite them being of no interest to any caller, and it being
vanishingly unlikely for a caller to appear which would care. Fine
for now, but I'd still like to understand why.

> +{
> +    for ( ; ; frag++, name++ )
> +    {
> +        unsigned char f = *frag, n = *name;
> +        int res = f - n;
> +
> +        if ( res || n == '\0' )
> +        {
> +            /*
> +             * NUL in 'name' matching a comma, colon or semicolon in 'frag'
> +             * implies success.
> +             */
> +            if ( n == '\0' && (f == ',' || f == ':' || f == ';') )
> +                res = 0;
> +
> +            return res;
> +        }
> +    }
> +}

Down the road I think we should consider further uses of the function
(in the generic parsing routines) and then whether to treat - and _
equally in command line options. Even more so with such an extension,
but already with the behavior above I wonder whether "strcmp" as
part of the name is really appropriate; cmdline_compare() or some such
might be more to the point, and then retaining the return type just to
match up with strcmp() would also disappear as an argument.

Anyway, for the immediate purpose
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
with or without the Arm related adjustment (decision there is with the
Arm maintainers anyway).

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel