[parted-devel] Re: bug? mac.c's mac_write modifies its *disk parameter

[Andrew Clausen]

Hi Jim,

If ped_disk_write() fails for some reason (eg: bad sectors), and has to
change information (like locations of data), then in-memory data might
need to change.  (Or, imagine a time-stamp...)

So, I think it should not have const in it.

In the mac_write() case, I suppose we could just PED_ASSERT the
existence of the partition map, and make sure all functions that
create/modify partition tables exit with the partition map in tact.
But, this might make it difficult to move the partition map to a
different location... not that the existing arrangement is brilliant.

Cheers,
Andrew


On Mon, Mar 05, 2007 at 09:51:16AM +0100, Jim Meyering wrote:
> I'm preparing a patch to add the "const" attribute where appropriate,
> starting with libparted/labels/dos.c.  This went fine (albeit
> tediously) up until the "msdos_write" function, which is tied to the
> _PedDiskOps.write member:
> 
>     int (*write) (PedDisk* disk);
> 
> Of course, like any write function, it should have a prototype,
> indicating that it treats its parameter as read-only:
> 
>     int (*write) (const PedDisk* disk);
> 
> so I changed it.  That change cascades to every other libparted/labels/*.c
> file, since they all have a similar FOO_write function.  And in many
> of those other files, the FOO_write function called some other static
> function with a non-const-but-should-be-const *disk, so I updated
> those, too.
> 
> This process seems to have exposed a logic error.  Unlike every other
> FOO_write function, mac.c's mac_write function, calls the
> "*disk"-modifying ped_disk_add_partition:
> 
> static int
> mac_write (PedDisk* disk)
> {
> 	if (!ped_disk_get_partition (disk, mac_disk_data->part_map_entry_num)) {
> 		if (!_disk_add_part_map_entry (disk, 1))
> 			goto error;
> 	}
> 
> The _disk_add_part_map_entry function also modified "*disk",
> so it doesn't belong here either.
> 
> For now, I've isolated mac.c with a single cast:
> 
> 	write:			(int (*) (const PedDisk*)) mac_write,
> 
> I.e., I am taking the conservative approach for now and leaving
> mac.c otherwise unchanged.
> 
> Can anyone say if there's a good reason to make such an exception?