Message ID | 1445270025-22999-33-git-send-email-mreitz@redhat.com |
---|---|
State | New |
Headers | show |
Am 19.10.2015 um 17:53 hat Max Reitz geschrieben: > Implement 'eject' by calling blockdev-open-tray and > blockdev-remove-medium. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > blockdev.c | 11 +++++------ > 1 file changed, 5 insertions(+), 6 deletions(-) > > diff --git a/blockdev.c b/blockdev.c > index a4c278f..0481686 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -1941,16 +1941,15 @@ out: > > void qmp_eject(const char *device, bool has_force, bool force, Error **errp) > { > - BlockBackend *blk; > + Error *local_err = NULL; > > - blk = blk_by_name(device); > - if (!blk) { > - error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, > - "Device '%s' not found", device); > + qmp_blockdev_open_tray(device, has_force, force, &local_err); > + if (local_err) { > + error_propagate(errp, local_err); > return; > } This changes the behaviour, in the current patch series in two ways if the device is locked: 1. With force, the qmp_blockdev_remove_medium() call will fail because we only unlocked the device, but didn't actually open the tray (I commented on this in the qmp_blockdev_open_tray() patch). This breaks the API, obviously. 2. Without force, eject previously sent an eject request and also returned a "Device is locked" error. Now it fails with "Tray of device is not open". Regression in the message quality, but not an API breakage because tools must not parse the message. > - eject_device(blk, force, errp); > + qmp_blockdev_remove_medium(device, errp); > } Kevin
On 23.10.2015 15:54, Kevin Wolf wrote: > Am 19.10.2015 um 17:53 hat Max Reitz geschrieben: >> Implement 'eject' by calling blockdev-open-tray and >> blockdev-remove-medium. >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> blockdev.c | 11 +++++------ >> 1 file changed, 5 insertions(+), 6 deletions(-) >> >> diff --git a/blockdev.c b/blockdev.c >> index a4c278f..0481686 100644 >> --- a/blockdev.c >> +++ b/blockdev.c >> @@ -1941,16 +1941,15 @@ out: >> >> void qmp_eject(const char *device, bool has_force, bool force, Error **errp) >> { >> - BlockBackend *blk; >> + Error *local_err = NULL; >> >> - blk = blk_by_name(device); >> - if (!blk) { >> - error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, >> - "Device '%s' not found", device); >> + qmp_blockdev_open_tray(device, has_force, force, &local_err); >> + if (local_err) { >> + error_propagate(errp, local_err); >> return; >> } > > This changes the behaviour, in the current patch series in two ways if > the device is locked: > > 1. With force, the qmp_blockdev_remove_medium() call will fail because > we only unlocked the device, but didn't actually open the tray (I > commented on this in the qmp_blockdev_open_tray() patch). This breaks > the API, obviously. Yep, will fix. > 2. Without force, eject previously sent an eject request and also > returned a "Device is locked" error. Now it fails with "Tray of > device is not open". Regression in the message quality, but not an > API breakage because tools must not parse the message. I think this should be fine. The previous message wasn't too good in my opinion either, because the most likely scenario is this: User issues eject, management tool reports qemu's message: "Device is locked!" and then the tray opens. So that's strange, too. Maybe "Tray of device is not open" is actually the better message here, I don't know. It better describes the state, but it does not describe the reason... But in addition to that, there is no easy way around this. I could put a check into qmp_eject() which returns a "Device is locked" message if the tray is still closed after a successful qmp_blockdev_open_tray(), but I don't know whether that's worth it. Max >> - eject_device(blk, force, errp); >> + qmp_blockdev_remove_medium(device, errp); >> } > > Kevin >
diff --git a/blockdev.c b/blockdev.c index a4c278f..0481686 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1941,16 +1941,15 @@ out: void qmp_eject(const char *device, bool has_force, bool force, Error **errp) { - BlockBackend *blk; + Error *local_err = NULL; - blk = blk_by_name(device); - if (!blk) { - error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, - "Device '%s' not found", device); + qmp_blockdev_open_tray(device, has_force, force, &local_err); + if (local_err) { + error_propagate(errp, local_err); return; } - eject_device(blk, force, errp); + qmp_blockdev_remove_medium(device, errp); } void qmp_block_passwd(bool has_device, const char *device,
Implement 'eject' by calling blockdev-open-tray and blockdev-remove-medium. Signed-off-by: Max Reitz <mreitz@redhat.com> --- blockdev.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)