Re: [Qemu-devel] [v9 Patch 3/6]Qemu: Cmd "block_set_hostcache" for dynamic cache change

[supriya kannery]

Luiz Capitulino wrote:
> On Fri, 11 Nov 2011 12:17:48 +0530
> Supriya Kannery <address@hidden> wrote:
>
>   
> > +
> > +    ret = bdrv_open(bs, bs->filename, bdrv_flags, drv);
> > +    if (ret < 0) {
> > +        /* Reopen failed. Try to open with original flags */
> > +        qerror_report(QERR_REOPEN_FILE_FAILED, bs->filename);
> > +        ret = bdrv_open(bs, bs->filename, open_flags, drv);
> > +        if (ret < 0) {
> > +            /* Reopen failed with orig and modified flags */
> > +            abort();
> > +        }
> > +    }
> > +
> > +    return ret;
> > +}
> >     
>
> In this thread:
>
>  http://lists.gnu.org/archive/html/qemu-devel/2011-11/msg01271.html
>
> Juan uses a similar method (well, at least it looks similar to me)
> to fix a problem with migration. However, it was said that that method
> can cause problems with -snapshot and encrypted images.
>
> Won't we have the same sort of problems with this series?
>
>
>   

Thanks! for this link. Yes, this series also need to look at such issues.
Will go through the discussions and see what needs to be looked at.

> > +    if (bdrv_is_inserted(bs)) {
> > +        /* Reopen file with changed set of flags */
> > +        return bdrv_reopen(bs, bdrv_flags);
> > +    } else {
> > +        /* Save hostcache change for future use */
> > +        bs->open_flags = bdrv_flags;
> > +        return 0;
> > +    }
> >     
>
> I'm wondering if the simplest (and best) thing to do here is to fail
> if the drive is not inserted. Just wondering, not exactly asking you
> to change it. But it should at least be clearly documented if you keep it.
>
>   

I initially started the patchset with returning error message on 
detecting uninserted drive.
But later thought that, user should be given the flexibility of changing 
hostcache setting
irrespective of whether drive is full or not. To what I understand, 
hostcache setting is
independent of what user puts in to the drive.

Need to make these saved status flags used when opening a newly inserted 
device.

> > +    /* Read hostcache setting */
> > +    enable = qdict_get_bool(qdict, "option");
> > +    return bdrv_change_hostcache(bs, enable);
> > +
> > +}
> >     
>
> This is not using the QAPI.
>   

will change to use QAPI.
>   
> > +
> > Index: qemu/blockdev.h
> > ===================================================================
> > --- qemu.orig/blockdev.h
> > +++ qemu/blockdev.h
> > @@ -65,5 +65,7 @@ int do_change_block(Monitor *mon, const 
> >  int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data);
> >  int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, QObject **ret_data);
> >  int do_block_resize(Monitor *mon, const QDict *qdict, QObject **ret_data);
> > +int do_block_set_hostcache(Monitor *mon, const QDict *qdict,
> > +                           QObject **ret_data);
> >  
> >  #endif
> > Index: qemu/hmp-commands.hx
> > ===================================================================
> > --- qemu.orig/hmp-commands.hx
> > +++ qemu/hmp-commands.hx
> > @@ -70,6 +70,20 @@ but should be used with extreme caution.
> >  resizes image files, it can not resize block devices like LVM volumes.
> >  ETEXI
> >  
> > +    {
> > +        .name       = "block_set_hostcache",
> > +        .args_type  = "device:B,option:b",
> >     
>
> "option" is not good enough. I'd call it "enable" or "disable".
>
>   

ok