Don Slutz <dsl...@verizon.com> writes:
> The other callers to blk_set_enable_write_cache() in this file
> already check for s->blk == NULL.
>
> Signed-off-by: Don Slutz <dsl...@verizon.com>
> ---
>
> I think this is a bugfix that should be back ported to stable
> releases.
>
> I also think this should be done in xen's copy of QEMU for 4.5 with
> back port(s) to active stable releases.
>
> Note: In 2.1 and earlier the routine is
> bdrv_set_enable_write_cache(); variable is s->bs.
Got a reproducer?
I'm asking because I believe s->identify_set implies s->blk.
s->identify_set is initialized to zero, and gets set to non-zero exactly
on the first successful IDENTIFY DEVICE or IDENTIFY PACKET DEVICE, in
ide_identify(), ide_atapi_identify() or ide_cfata_identify(),
respectively. Only called via cmd_identify() / cmd_identify_packet()
via ide_exec_cmd(). The latter immediately fails when !s->blk:
s = idebus_active_if(bus);
/* ignore commands to non existent slave */
if (s != bus->ifs && !s->blk) {
return;
}
Even if I'm right, your patch is fine, because it makes this spot more
obviously correct, and consistent with the other uses of
blk_set_enable_write_cache(). The case for stable is weak, though.
>
> hw/ide/core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 00e21cf..d4af5e2 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -2401,7 +2401,7 @@ static int ide_drive_post_load(void *opaque, int
> version_id)
> {
> IDEState *s = opaque;
>
> - if (s->identify_set) {
> + if (s->blk && s->identify_set) {
> blk_set_enable_write_cache(s->blk, !!(s->identify_data[85] & (1 <<
> 5)));
> }
> return 0;
