Re: [Qemu-devel] [PATCH V2 10/10] hw/sd.c: introduce SD card "image" property and allow SD hot-insert

[Luiz Capitulino]

On Thu, 05 Apr 2012 19:02:23 +0200
Paolo Bonzini <address@hidden> wrote:

> Il 05/04/2012 17:48, Igor Mitsyanko ha scritto:
> > New SD card "image" property can be used to:
> > - change image file attached to virtual SD card
> > - hot-insert new image file into newly initialized BlockDriverState (this 
> > was not
> > possible before).
> > 
> > Example usage:
> > ./qom-set /machine/milkymist/milkymist-memcard/card.image 
> > /home/me/mynewcard.img
> > this will attach image file /home/me/mynewcard.img to virtual SD card 
> > connected to
> > milkymist-memcard host controller device. If virtual card was already 
> > attached to
> > some other image file, eject event is triggered before attaching new file.
> > 
> > Signed-off-by: Igor Mitsyanko <address@hidden>
> > ---
> >  hw/sd.c |   44 ++++++++++++++++++++++++++++++++++++++++++++
> >  1 files changed, 44 insertions(+), 0 deletions(-)
> > 
> > diff --git a/hw/sd.c b/hw/sd.c
> > index 8ffaa17..3e75405 100644
> > --- a/hw/sd.c
> > +++ b/hw/sd.c
> > @@ -1816,6 +1816,48 @@ static void sd_set_spimode(Object *obj, Visitor *v, 
> > void *opaque,
> >      }
> >  }
> >  
> > +static void sd_set_image_path(Object *obj, Visitor *v, void *opaque,
> > +                         const char *name, Error **errp)
> > +{
> > +    SDState *sd = SD_CARD(obj);
> > +    char *new_image;
> > +
> > +    visit_type_str(v, &new_image, "file", errp);
> 
> Please use the name argument instead of "file".
> 
> > +
> > +    if (error_is_set(errp)) {
> > +        return;
> > +    }
> > +
> > +    if (sd->bdrv) {
> > +        qmp_change_blockdev(bdrv_get_device_name(sd->bdrv), new_image,
> > +                false, NULL, errp);
> > +    } else {
> > +        DriveInfo *di;
> > +        QemuOpts *opts = drive_add(IF_SD, sd->if_idx, new_image, "");
> 
> I think this should simply take a drive name and pass it to
> bdrv_get_device_name.  The drive_add/drive_init should be done
> separately with the drive_add monitor command, like
> 
> drive_add 0 file=foo.img,if=none,id=bar
> 
> With the upcoming support for static properties in objects that are not
> devices, you can just add a drive property to the SD class.
> 
> There is a problem here however.  QOM commands are HMP only, and
> drive_add is QMP only.

I think you meant QOM commands are QMP only, and drive_add is HMP only?

>  I think that blocking drive_add in QMP is
> perfect being the enemy of good.  Alternatively, however, adding the QOM
> property commands to HMP would also make sense.

We've agreed on converting all existing HMP-only commands, such as drive_add,
to QMP as they exist today. No problem adding drive_add to QMP then.