[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 2/3] libparted: make pc98 detection depend on signatures (#64
From: |
Jim Meyering |
Subject: |
Re: [PATCH 2/3] libparted: make pc98 detection depend on signatures (#646053) |
Date: |
Sat, 15 Oct 2011 11:10:38 +0200 |
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.