Re: [PATCH 3/3] grub EFI disk device enumberating

[Johan Rydberg]

"bibo,mao" <address@hidden> writes:

>  Previously I posted this patch, now I repost again.
>  Any suggestion is welcome.

I will just give a comment here.  Please do not treat it as a full
review, I'll leave that to Okuji.

> +static int compare_ancestor_path(const grub_efi_device_path_t *parent,
> +                const grub_efi_device_path_t *dp2)
> +{
> [...]
> +  while (1)
> +    {
> +      grub_efi_uint8_t type1, type2;
> +      grub_efi_uint8_t subtype1, subtype2;
> +      grub_efi_uint16_t len1, len2;
> +      int ret;
> +
> +      if (GRUB_EFI_END_ENTIRE_DEVICE_PATH(parent))
> +     break;
> +
> +      type1 = GRUB_EFI_DEVICE_PATH_TYPE (parent);
> +      type2 = GRUB_EFI_DEVICE_PATH_TYPE (dp2);
> +
> +      if (type1 != type2)
> +     return (int) type2 - (int) type1;
> +
> +      subtype1 = GRUB_EFI_DEVICE_PATH_SUBTYPE (parent);
> +      subtype2 = GRUB_EFI_DEVICE_PATH_SUBTYPE (dp2);
> +
> +      if (subtype1 != subtype2)
> +     return (int) subtype1 - (int) subtype2;
> +
> +      len1 = GRUB_EFI_DEVICE_PATH_LENGTH (parent);
> +      len2 = GRUB_EFI_DEVICE_PATH_LENGTH (dp2);
> +
> +      if (len1 != len2)
> +     return (int) len1 - (int) len2;
> +
> +      ret = grub_memcmp (parent, dp2, len1);
> +      if (ret != 0)
> +     return ret;
> +
> +      parent = (grub_efi_device_path_t *) ((char *) parent + len1);
> +      dp2 = (grub_efi_device_path_t *) ((char *) dp2 + len2);

This code can be shorter; you only have to compare the lengths. If
they match, you can do a memcmp on the whole device path.  

Also, can you guarantee that the child path "dp2" is longer than the
parent path?  What it does is that it checks if "parent" is a subpath
of "dp2", so a function name reflecting that probably fits better.
Name functions after what they do, not what they are used to.  Better
argument names are also welcome.

~j

pgpNI5A61xLwr.pgp
Description: PGP signature