| Message ID | 1465242006-22509-1-git-send-email-clord@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Am 06.06.2016 um 21:40 hat Colin Lord geschrieben: > This commit causes qmp_blockdev_change_medium to report an error if an > attempt is made to open a device with a locked tray. The old behaviour is that the command seemingly succeeds, but the medium isn't actually changed. Correct? Should this be mentioned in the commit message? You just describe what you change, but not why. > Signed-off-by: Colin Lord <clord@redhat.com> > This is based off my previous patch regarding the do_open_tray function > (currently at v3). Probably should have been submitted as a patch set > but I wasn't thinking that far ahead when I submitted the first patch. > --- > blockdev.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) Yes, would probably have made sense as a series, but as long as it's only two patches, it's not really a problem. Please make sure to put such comments below the "---" line, though, i.e. comments that make sense for the review, but not as part of the commit log. Then git-am automatically removes that part from the commit message while applying the patch. I did it manually for this one now. Kevin
On 06/07/2016 06:28 AM, Kevin Wolf wrote: > Am 06.06.2016 um 21:40 hat Colin Lord geschrieben: >> This commit causes qmp_blockdev_change_medium to report an error if an >> attempt is made to open a device with a locked tray. > > The old behaviour is that the command seemingly succeeds, but the medium > isn't actually changed. Correct? > Close. Old "change" command also fails, but with a confusing error. > Should this be mentioned in the commit message? You just describe what > you change, but not why. > Old behavior: - Change uses qmp_blockdev_open_tray, which "succeeds." - Change then tries to use qmp_x_blockdev_remove_medium, but receives potentially confusing error "Tray is locked." - Moments later, the tray is likely now open. New behavior: - Change uses do_open_tray, which returns -EINPROGRESS. - Change can propagate this error upwards without attempting to remove the medium. - User gets "Device <foo> is locked and force was not specified, wait for tray to open and try again" error. Why: "The new error tries to inform the user that there is an action pending and that the command, if run again, may succeed." >> Signed-off-by: Colin Lord <clord@redhat.com> >> This is based off my previous patch regarding the do_open_tray function >> (currently at v3). Probably should have been submitted as a patch set >> but I wasn't thinking that far ahead when I submitted the first patch. >> --- >> blockdev.c | 7 +++++-- >> 1 file changed, 5 insertions(+), 2 deletions(-) > > Yes, would probably have made sense as a series, but as long as it's > only two patches, it's not really a problem. > > Please make sure to put such comments below the "---" line, though, i.e. > comments that make sense for the review, but not as part of the commit > log. Then git-am automatically removes that part from the commit message > while applying the patch. I did it manually for this one now. > > Kevin >
Am 07.06.2016 um 22:00 hat John Snow geschrieben: > > > On 06/07/2016 06:28 AM, Kevin Wolf wrote: > > Am 06.06.2016 um 21:40 hat Colin Lord geschrieben: > >> This commit causes qmp_blockdev_change_medium to report an error if an > >> attempt is made to open a device with a locked tray. > > > > The old behaviour is that the command seemingly succeeds, but the medium > > isn't actually changed. Correct? > > > > Close. Old "change" command also fails, but with a confusing error. > > > Should this be mentioned in the commit message? You just describe what > > you change, but not why. > > > > Old behavior: > > - Change uses qmp_blockdev_open_tray, which "succeeds." > - Change then tries to use qmp_x_blockdev_remove_medium, but receives > potentially confusing error "Tray is locked." > - Moments later, the tray is likely now open. Ah, yes, makes sense. This and that we want to have a less confusing error message is the part that is missing in the commit message. Kevin > New behavior: > > - Change uses do_open_tray, which returns -EINPROGRESS. > - Change can propagate this error upwards without attempting to remove > the medium. > - User gets "Device <foo> is locked and force was not specified, wait > for tray to open and try again" error. > > Why: "The new error tries to inform the user that there is an action > pending and that the command, if run again, may succeed." > > >> Signed-off-by: Colin Lord <clord@redhat.com> > >> This is based off my previous patch regarding the do_open_tray function > >> (currently at v3). Probably should have been submitted as a patch set > >> but I wasn't thinking that far ahead when I submitted the first patch. > >> --- > >> blockdev.c | 7 +++++-- > >> 1 file changed, 5 insertions(+), 2 deletions(-) > > > > Yes, would probably have made sense as a series, but as long as it's > > only two patches, it's not really a problem. > > > > Please make sure to put such comments below the "---" line, though, i.e. > > comments that make sense for the review, but not as part of the commit > > log. Then git-am automatically removes that part from the commit message > > while applying the patch. I did it manually for this one now. > > > > Kevin > > > > -- > —js
diff --git a/blockdev.c b/blockdev.c index 7dd14b9..8a045d9 100644 --- a/blockdev.c +++ b/blockdev.c @@ -2544,6 +2544,7 @@ void qmp_blockdev_change_medium(const char *device, const char *filename, BlockBackend *blk; BlockDriverState *medium_bs = NULL; int bdrv_flags; + int rc; QDict *options = NULL; Error *err = NULL; @@ -2598,11 +2599,13 @@ void qmp_blockdev_change_medium(const char *device, const char *filename, goto fail; } - qmp_blockdev_open_tray(device, false, false, &err); - if (err) { + rc = do_open_tray(device, false, &err); + if (rc && rc != -ENOSYS) { error_propagate(errp, err); goto fail; } + error_free(err); + err = NULL; qmp_x_blockdev_remove_medium(device, &err); if (err) {
This commit causes qmp_blockdev_change_medium to report an error if an attempt is made to open a device with a locked tray. Signed-off-by: Colin Lord <clord@redhat.com> This is based off my previous patch regarding the do_open_tray function (currently at v3). Probably should have been submitted as a patch set but I wasn't thinking that far ahead when I submitted the first patch. --- blockdev.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)