[parted-devel] [PATCH 1/2] do not discard bootcode from extended partition

[Joel Granados]

On Tue, Aug 25, 2009 at 01:08:22PM +0200, Petr Uzel wrote:
> 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

I'd prefer b.

> 
> 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: puzel at suse.cz
> Lihovarská 1060/12                          http://www.suse.cz
> 190 00 Prague 9, CR                             



> _______________________________________________
> parted-devel mailing list
> parted-devel at lists.alioth.debian.org
> http://lists.alioth.debian.org/mailman/listinfo/parted-devel

-- 
Joel Andres Granados
Brno, Czech Republic, Red Hat.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: <http://lists.alioth.debian.org/pipermail/parted-devel/attachments/20090826/f969d856/attachment.pgp>