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

[Jim Meyering]

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.
...
> 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.