| Message ID | 1333640913-16028-11-git-send-email-i.mitsyanko@samsung.com |
|---|---|
| State | New |
| Headers | show |
On Thu, 05 Apr 2012 19:02:23 +0200 Paolo Bonzini <pbonzini@redhat.com> 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 <i.mitsyanko@samsung.com> > > --- > > 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.
On 04/05/2012 09:02 PM, Paolo Bonzini 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<i.mitsyanko@samsung.com> >> --- >> 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". > OK >> + >> + 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. Did you mean bdrv_find()? I can't use it here, BlockDriverState doesn't exist yet, I need to create it first with drive_add. > 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 > So you're saying we need to use two commands to insert a new virtual SD card: first we need to create a BlockDriverState with specified device_name and then with second command we associate SD card with previously created BlockDriverState? I kind of like another approach, when we can just set filename as SD card's property and BlockDriverState will be automatically created and connected to this SD card's state. And when we want to eject card from virtual slot, we set SDCard::eject property to true and then associated BlockDriverState is dereferenced and freed automatically. > 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 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. I don't understand, is it some kind of conceptual restriction? We just call visit_type_str() here, it could be called by object_property_set_str(), or from QMP socket, or from monitor for all we care, what's the difference?
Il 09/04/2012 16:30, Igor Mitsyanko ha scritto: >>> + >>> + 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. > Did you mean bdrv_find()? I can't use it here, BlockDriverState doesn't > exist yet, I need to create it first with drive_add. Yes, that's the point. :) >> 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 >> > So you're saying we need to use two commands to insert a new virtual SD > card: first we need to create a BlockDriverState with specified device_name > and then with second command we associate SD card with previously created > BlockDriverState? > I kind of like another approach, when we can just set filename as SD > card's property and BlockDriverState will be automatically created and > connected to this SD card's state. But then you cannot set any property like the format, the caching mode, etc. Especially with the default of cache=writethrough it may be *very* slow. > And when we want to eject card from virtual slot, we set SDCard::eject > property to true and then associated BlockDriverState is dereferenced > and freed automatically. That's possible, though I'm not sure why the normal eject/change commands cannot work here. >> There is a problem here however. QOM commands are HMP only, and >> drive_add is QMP 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. > I don't understand, is it some kind of conceptual restriction? No, it's just that nobody implemented them. Paolo
Il 09/04/2012 14:42, Luiz Capitulino ha scritto: >> > >> > 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? Yes. >> > 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. Cool, thanks! Paolo
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); + + 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, ""); + + if (!opts) { + error_set(errp, QERR_OPEN_FILE_FAILED, new_image); + return; + } + + di = drive_init(opts, 0); + if (!di) { + error_set(errp, QERR_OPEN_FILE_FAILED, new_image); + return; + } + + sd_reset(sd, di->bdrv); + if (bdrv_attach_dev(sd->bdrv, sd) < 0) { + drive_put_ref(di); + error_set(errp, QERR_OPEN_FILE_FAILED, new_image); + return; + } + bdrv_set_dev_ops(sd->bdrv, &sd_block_ops, sd); + qemu_set_irq(sd->inserted_cb, bdrv_is_inserted(sd->bdrv)); + qemu_set_irq(sd->readonly_cb, sd->wp_switch); + } +} + static void sd_initfn(Object *obj) { SDState *sd = SD_CARD(obj); @@ -1826,6 +1868,8 @@ static void sd_initfn(Object *obj) NULL, NULL, NULL); object_property_add(obj, "spi-mode", "boolean", sd_is_spi, sd_set_spimode, NULL, NULL, NULL); + object_property_add(OBJECT(sd), "image", "string", + NULL, sd_set_image_path, NULL, NULL, NULL); } static TypeInfo sd_type_info = {
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 <i.mitsyanko@samsung.com> --- hw/sd.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 44 insertions(+), 0 deletions(-)