Re: [Qemu-devel] [PATCH V2 2/3] hw/sd.c: add SD card save/load support

[Peter Maydell]

On 28 December 2011 12:08, Mitsyanko Igor <address@hidden> wrote:
> We couldn't properly implement save/restore functionality of SD host 
> controllers
> states without SD card's state VMStateDescription implementation. This patch
> updates SD card emulation to support save/load of card's state. Update 
> requires
> changing of data type of several variables in SDState. Variables order 
> rearranged
> to ensure proper data alignment in SDState structure.
> For consistency, because several variables now have bool datatype, API was 
> modified
> to use bool as well, 0 was changed to 'false' and 1 was changed to 'true' in 
> those
> places where it was appropriate.

If you're going to switch things to bool, can you break those out
into separate patches for the individual things you're changing,
please? Otherwise this patch is trying to do too many things
at once.

Also, why should we care particularly about the order of
fields in SDState? There will be at most a handful of copies
of this struct in qemu, costing a handful of bytes in extra
padding. ("ensure proper data alignment" is wrong -- the compiler
does this for us.) If you feel you must rearrange things, again,
please put it in a separate patch so it's easier to read.

> @@ -415,14 +418,14 @@ static void sd_reset(SDState *sd, BlockDriverState 
> *bdrv)
>     if (sd->wp_groups)
>         g_free(sd->wp_groups);
>     sd->wp_switch = bdrv ? bdrv_is_read_only(bdrv) : 0;
> -    sd->wp_groups = (int *) g_malloc0(sizeof(int) * sect);
> -    memset(sd->function_group, 0, sizeof(int) * 6);
> +    sd->wp_groups = (bool *)g_malloc0(sizeof(bool) * sect);

 sd->wp_groups = g_new0(bool, sect);

-- PMM