On Fri, Aug 21, 2009 at 05:09:00PM +0200, Jim Meyering wrote: > Petr Uzel wrote: > > * libparted/labels/dos.c (write_ext_table): Do not discard > > bootcode from extended partition on msdos label when some of > > the logical partitions are changed > > Hi Petr, > Thanks for working on this.
Hi Jim,
Thanks for the comments - I agree with all of them.
Since the patch has been already applied, the question is, whether
I should
a) revert the applied patch and post a new one addressing the issues
b) leave the applied patch and fix the issues in other patches
In the latter case I don't know how to fix the changelog.
So which way do you prefer?
> ...
> > diff --git a/libparted/labels/dos.c b/libparted/labels/dos.c
> > index 1d4c2dd..7b0c6d6 100644
> > --- a/libparted/labels/dos.c
> > +++ b/libparted/labels/dos.c
> > @@ -1027,7 +1027,8 @@ write_ext_table (const PedDisk* disk,
> > PedSector sector, const PedPartition* logical)
> > {
> > PedPartition* part;
> > - PedSector lba_offset;
> > + PedSector lba_offset;
> > + void* s;
>
> Please do not modify unrelated lines like the lba_offset declaration.
> Also, please leave the declaration of "s" back just before the first use.
> That helps readability/maintainability, since then, there is
> no risk that someone will use it between the declaration and first use.
>
> >
> > PED_ASSERT (disk != NULL, return 0);
> > PED_ASSERT (ped_disk_extended_partition (disk) != NULL, return 0);
> > @@ -1035,10 +1036,11 @@ write_ext_table (const PedDisk* disk,
> >
> > lba_offset = ped_disk_extended_partition (disk)->geom.start;
> >
> > - void *s = ped_calloc (disk->dev->sector_size);
> > - if (s == NULL)
> > + if (!ptt_read_sector (disk->dev, sector, &s))
> > return 0;
> > +
> > DosRawTable *table = s;
> > + memset(&(table->partitions), 0, 4 * sizeof(DosRawPartition));
>
> Please don't hard-code constants like that.
> The 3rd argument above should be "sizeof (table->partitions)".
>
> > table->magic = PED_CPU_TO_LE16 (MSDOS_MAGIC);
> >
> > int ok = 0;
> > @@ -1073,10 +1075,15 @@ static int
>
> Your log doesn't mention this function.
> Yet it is no longer officially writing an empty table.
> Such a change in semantics deserves an explanatory comment
> in the code as well as mention in the log.
>
> > write_empty_table (const PedDisk* disk, PedSector sector)
> > {
> > DosRawTable table;
> > + void* table_sector;
> >
> > PED_ASSERT (disk != NULL, return 0);
> >
> > - memset (&table, 0, sizeof (DosRawTable));
> > + if (ptt_read_sector (disk->dev, sector, &table_sector)) {
> > + memcpy (&table, table_sector, sizeof(DosRawTable));
>
> Please do not use "sizeof(TYPE)" when you can instead use
> "sizeof variable". The latter is more maintainable.
> Here, you should use "sizeof table".
>
> > + free(table_sector);
> > + }
> > + memset (&(table.partitions), 0, 4 * sizeof(DosRawPartition));
>
> Don't hard-code 4, as above.
--
Best regards / s pozdravem
Petr Uzel, openSUSE Community Multiplier Team
-----------------------------------------------------------------
SUSE LINUX, s.r.o. e-mail: [email protected]
Lihovarská 1060/12 http://www.suse.cz
190 00 Prague 9, CR
pgpp5qOWXvhuJ.pgp
Description: PGP signature
_______________________________________________ parted-devel mailing list [email protected] http://lists.alioth.debian.org/mailman/listinfo/parted-devel
