Re: [PATCH 2/3] libparted: make pc98 detection depend on signatures (#646053)

[Jim Meyering]

Brian C. Lane wrote:
> From: "Brian C. Lane" <address@hidden>
>
> pc98 is not a common disk label. Change pc98_probe to only return true
> if one of the recognized signatures is present.
> Currently these include:
>
> IPL1
> Linux 98
> GRUB/98
>
> This will prevent false-positive detection on msdos labeled disks
>
> * libparted/labels/pc98.c (pc98_probe): Change to require signature
>   (pc98_check_ipl_signature): Add more signatures
> ---
>  libparted/labels/pc98.c |   32 ++++++++++----------------------
>  1 files changed, 10 insertions(+), 22 deletions(-)
>
> diff --git a/libparted/labels/pc98.c b/libparted/labels/pc98.c
> index 3afa8a2..ea3cf4e 100644
> --- a/libparted/labels/pc98.c
> +++ b/libparted/labels/pc98.c
> @@ -140,7 +140,14 @@ pc98_check_magic (const PC98RawTable *part_table)
>  static int
>  pc98_check_ipl_signature (const PC98RawTable *part_table)
>  {
> -     return !memcmp (part_table->boot_code + 4, "IPL1", 4);
> +     if (memcmp (part_table->boot_code + 4, "IPL1", 4) == 0)
> +             return 1;
> +     else if (memcmp (part_table->boot_code + 4, "Linux 98", 8) == 0)
> +             return 1;
> +     else if (memcmp (part_table->boot_code + 4, "GRUB/98 ", 8) == 0)
> +             return 1;
> +     else
> +             return 0;
>  }
>
>  static int
> @@ -192,27 +199,8 @@ pc98_probe (const PedDevice *dev)
>       if (!pc98_check_magic (&part_table))
>               return 0;
>
> -     /* check consistency */
> -     empty = 1;
> -     for (p = part_table.partitions;
> -          p < part_table.partitions + MAX_PART_COUNT;
> -          p++)
> -     {
> -             if (p->mid == 0 && p->sid == 0)
> -                     continue;
> -             empty = 0;
> -             if (!check_partition_consistency (dev, p))
> -                     return 0;
> -     }
> -
> -     /* check boot loader */
> -     if (pc98_check_ipl_signature (&part_table))
> -             return 1;
> -     else if (part_table.boot_code[0])       /* invalid boot loader */
> -             return 0;
> -
> -     /* Not to mistake msdos disk map for PC-9800's empty disk map  */
> -     if (empty)
> +     /* check for boot loader signatures */
> +     if (!pc98_check_ipl_signature (&part_table))
>               return 0;
>
>       return 1;

Hi Brian,

Thanks for the patch.
However, do you really intend to remove those check_partition_consistency
calls?  That seems like it would weaken the test unnecessarily.

Also, would you please change the name to pc98_valid_ipl_signature?
That will make it more readable for me.  With the "_check_" name,
I find that I have to read the code to determine the semantics.
While with "_valid_...", it's obviously a boolean.
And then you can change the last four lines to be simply this:

    return pc98_valid_ipl_signature (&part_table);

Finally, please always configure with

    ./configure --enable-gcc-warnings

That would have highlighted some unused variables and the unused function.