| Message ID | 1319540020-32484-7-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
Am 25.10.2011 12:53, schrieb Paolo Bonzini: > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > block.c | 7 +++++++ > block.h | 7 +++++++ > blockdev.c | 8 +++++--- > 3 files changed, 19 insertions(+), 3 deletions(-) > > diff --git a/block.c b/block.c > index 9873b57..53e21ba 100644 > --- a/block.c > +++ b/block.c > @@ -821,6 +821,13 @@ bool bdrv_dev_has_removable_media(BlockDriverState *bs) > return !bs->dev || (bs->dev_ops && bs->dev_ops->change_media_cb); > } > > +void bdrv_dev_eject_request(BlockDriverState *bs, bool force) > +{ > + if (bs->dev_ops && bs->dev_ops->eject_request_cb) { > + bs->dev_ops->eject_request_cb(bs->dev_opaque, force); > + } > +} > + > bool bdrv_dev_is_tray_open(BlockDriverState *bs) > { > if (bs->dev_ops && bs->dev_ops->is_tray_open) { > diff --git a/block.h b/block.h > index e77988e..d3c3d62 100644 > --- a/block.h > +++ b/block.h > @@ -39,6 +39,12 @@ typedef struct BlockDevOps { > */ > void (*change_media_cb)(void *opaque, bool load); > /* > + * Runs when an eject request is issued from the monitor, the tray > + * is closed, and the medium is locked. > + * Device models with removable media must implement this callback. > + */ > + void (*eject_request_cb)(void *opaque, bool force); > + /* > * Is the virtual tray open? > * Device models implement this only when the device has a tray. > */ > @@ -116,6 +122,7 @@ void bdrv_detach_dev(BlockDriverState *bs, void *dev); > void *bdrv_get_attached_dev(BlockDriverState *bs); > void bdrv_set_dev_ops(BlockDriverState *bs, const BlockDevOps *ops, > void *opaque); > +void bdrv_dev_eject_request(BlockDriverState *bs, bool force); > bool bdrv_dev_has_removable_media(BlockDriverState *bs); > bool bdrv_dev_is_tray_open(BlockDriverState *bs); > bool bdrv_dev_is_medium_locked(BlockDriverState *bs); > diff --git a/blockdev.c b/blockdev.c > index 0827bf7..4cf333a 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -635,9 +635,11 @@ static int eject_device(Monitor *mon, BlockDriverState *bs, int force) > qerror_report(QERR_DEVICE_NOT_REMOVABLE, bdrv_get_device_name(bs)); > return -1; > } > - if (!force && !bdrv_dev_is_tray_open(bs) > - && bdrv_dev_is_medium_locked(bs)) { > - qerror_report(QERR_DEVICE_LOCKED, bdrv_get_device_name(bs)); > + if (bdrv_dev_is_medium_locked(bs) && !bdrv_dev_is_tray_open(bs)) { > + bdrv_dev_eject_request(bs, force); > + if (!force) { > + qerror_report(QERR_DEVICE_LOCKED, bdrv_get_device_name(bs)); > + } > return -1; > } > bdrv_close(bs); Now force doesn't force any more. It avoids the error message, but doesn't forcefully close the BlockDriverState any more. Intentional? If so, why is it a good idea? Kevin
On 10/28/2011 07:21 PM, Kevin Wolf wrote: >> > - if (!force&& !bdrv_dev_is_tray_open(bs) >> > -&& bdrv_dev_is_medium_locked(bs)) { >> > - qerror_report(QERR_DEVICE_LOCKED, bdrv_get_device_name(bs)); >> > + if (bdrv_dev_is_medium_locked(bs)&& !bdrv_dev_is_tray_open(bs)) { >> > + bdrv_dev_eject_request(bs, force); >> > + if (!force) { >> > + qerror_report(QERR_DEVICE_LOCKED, bdrv_get_device_name(bs)); >> > + } >> > return -1; >> > } >> > bdrv_close(bs); > Now force doesn't force any more. It avoids the error message, but > doesn't forcefully close the BlockDriverState any more. Intentional? If > so, why is it a good idea? In theory the guest OS should eject the disk itself. However, force does unlock the disk so that: 1) two ejects will have the desired effect; 2) force eject followed by change will work even with the tray locked, unlike before this series. Paolo
I apologize for the lateness of this review. Paolo Bonzini <pbonzini@redhat.com> writes: > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> The commit message should explain why we need this callback. The cover letter says "support for eject requests is required by udev 173." Please elaborate on that. > --- > block.c | 7 +++++++ > block.h | 7 +++++++ > blockdev.c | 8 +++++--- > 3 files changed, 19 insertions(+), 3 deletions(-) > > diff --git a/block.c b/block.c > index 9873b57..53e21ba 100644 > --- a/block.c > +++ b/block.c > @@ -821,6 +821,13 @@ bool bdrv_dev_has_removable_media(BlockDriverState *bs) > return !bs->dev || (bs->dev_ops && bs->dev_ops->change_media_cb); > } > > +void bdrv_dev_eject_request(BlockDriverState *bs, bool force) > +{ > + if (bs->dev_ops && bs->dev_ops->eject_request_cb) { > + bs->dev_ops->eject_request_cb(bs->dev_opaque, force); > + } > +} > + > bool bdrv_dev_is_tray_open(BlockDriverState *bs) > { > if (bs->dev_ops && bs->dev_ops->is_tray_open) { > diff --git a/block.h b/block.h > index e77988e..d3c3d62 100644 > --- a/block.h > +++ b/block.h > @@ -39,6 +39,12 @@ typedef struct BlockDevOps { > */ > void (*change_media_cb)(void *opaque, bool load); > /* > + * Runs when an eject request is issued from the monitor, the tray > + * is closed, and the medium is locked. > + * Device models with removable media must implement this callback. > + */ > + void (*eject_request_cb)(void *opaque, bool force); > + /* You implement it for IDE in PATCH 7/8 and SCSI in PATCH 8/8. You don't implement it for floppy, despite the comment. That's okay; floppy has no use for it. It's the comment that needs fixing. Devices that implement is_medium_locked() must implement this one as well. > * Is the virtual tray open? > * Device models implement this only when the device has a tray. > */ > @@ -116,6 +122,7 @@ void bdrv_detach_dev(BlockDriverState *bs, void *dev); > void *bdrv_get_attached_dev(BlockDriverState *bs); > void bdrv_set_dev_ops(BlockDriverState *bs, const BlockDevOps *ops, > void *opaque); > +void bdrv_dev_eject_request(BlockDriverState *bs, bool force); > bool bdrv_dev_has_removable_media(BlockDriverState *bs); > bool bdrv_dev_is_tray_open(BlockDriverState *bs); > bool bdrv_dev_is_medium_locked(BlockDriverState *bs); > diff --git a/blockdev.c b/blockdev.c > index 0827bf7..4cf333a 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -635,9 +635,11 @@ static int eject_device(Monitor *mon, BlockDriverState *bs, int force) > qerror_report(QERR_DEVICE_NOT_REMOVABLE, bdrv_get_device_name(bs)); > return -1; > } > - if (!force && !bdrv_dev_is_tray_open(bs) > - && bdrv_dev_is_medium_locked(bs)) { > - qerror_report(QERR_DEVICE_LOCKED, bdrv_get_device_name(bs)); > + if (bdrv_dev_is_medium_locked(bs) && !bdrv_dev_is_tray_open(bs)) { > + bdrv_dev_eject_request(bs, force); > + if (!force) { > + qerror_report(QERR_DEVICE_LOCKED, bdrv_get_device_name(bs)); > + } > return -1; > } > bdrv_close(bs); Like Kevin, I'm not entirely comfortable with changing the meaning of "-f". Here's my mental model of monitor command eject: 1. eject without -f behaves like the physical tray button. It has immediate effect, unless the tray is locked closed. Then, the drive just notifies the OS of the button push, so the OS can react to it. The latter isn't implemented in QEMU. 2. eject with -f behaves like whatever physical way there is to pry the tray open, locked or not. CD-ROM drives commonly have a little button hidden in some hope you can reach with a bent paperclip. Could you explain your mental model?
On 11/07/2011 02:21 PM, Markus Armbruster wrote: > The commit message should explain why we need this callback. The cover > letter says "support for eject requests is required by udev 173." > Please elaborate on that. Well, first and foremost eject requests are in the spec. :) The fact that recent guests need it is more a justification for pulling this in 1.0. > You implement it for IDE in PATCH 7/8 and SCSI in PATCH 8/8. You don't > implement it for floppy, despite the comment. That's okay; floppy has > no use for it. It's the comment that needs fixing. Devices that > implement is_medium_locked() must implement this one as well. Right. > 1. eject without -f behaves like the physical tray button. It has > immediate effect, unless the tray is locked closed. Then, the drive > just notifies the OS of the button push, so the OS can react to it. The > latter isn't implemented in QEMU. > > 2. eject with -f behaves like whatever physical way there is to pry the > tray open, locked or not. CD-ROM drives commonly have a little button > hidden in some hope you can reach with a bent paperclip. > > Could you explain your mental model? 1. eject without -f is as you mentioned. 2. eject with -f should really never be needed, but it does whatever is needed to be able to follow up with a "change" command. It turns out it is really "unlock" and "ask the guest to eject" combined, but that's the implementation, not the model. The difference from the paperclip model is that it gives a chance for the OS to clean up and eject safely. It wouldn't be hard to convince me otherwise though, especially if it can help getting the patch in 1.0. The "eject -f"+"change" can be replaced by "eject", possibly followed by "eject -f" after a timeout, and then followed again by "change". Paolo
Am 07.11.2011 14:36, schrieb Paolo Bonzini: > On 11/07/2011 02:21 PM, Markus Armbruster wrote: >> The commit message should explain why we need this callback. The cover >> letter says "support for eject requests is required by udev 173." >> Please elaborate on that. > > Well, first and foremost eject requests are in the spec. :) The fact > that recent guests need it is more a justification for pulling this in 1.0. > >> You implement it for IDE in PATCH 7/8 and SCSI in PATCH 8/8. You don't >> implement it for floppy, despite the comment. That's okay; floppy has >> no use for it. It's the comment that needs fixing. Devices that >> implement is_medium_locked() must implement this one as well. > > Right. > >> 1. eject without -f behaves like the physical tray button. It has >> immediate effect, unless the tray is locked closed. Then, the drive >> just notifies the OS of the button push, so the OS can react to it. The >> latter isn't implemented in QEMU. >> >> 2. eject with -f behaves like whatever physical way there is to pry the >> tray open, locked or not. CD-ROM drives commonly have a little button >> hidden in some hope you can reach with a bent paperclip. >> >> Could you explain your mental model? > > 1. eject without -f is as you mentioned. > > 2. eject with -f should really never be needed, but it does whatever is > needed to be able to follow up with a "change" command. It turns out it > is really "unlock" and "ask the guest to eject" combined, but that's the > implementation, not the model. Does this give different results than just asking the guest to eject without forcefully unlocking? I would expect that a guest that responds to the eject request would also unlock the drive. In which case I think eject without -f should be enough? > The difference from the paperclip model is that it gives a chance for > the OS to clean up and eject safely. It wouldn't be hard to convince me > otherwise though, especially if it can help getting the patch in 1.0. > The "eject -f"+"change" can be replaced by "eject", possibly followed by > "eject -f" after a timeout, and then followed again by "change". Kevin
On 11/07/2011 02:49 PM, Kevin Wolf wrote: > > 2. eject with -f should really never be needed, but it does whatever is > > needed to be able to follow up with a "change" command. It turns out it > > is really "unlock" and "ask the guest to eject" combined, but that's the > > implementation, not the model. > > Does this give different results than just asking the guest to eject > without forcefully unlocking? I would expect that a guest that responds > to the eject request would also unlock the drive. In which case I think > eject without -f should be enough? Only if the guest is not buggy (e.g. locks the tray but stops polling for eject requests) and has not crashed. Paolo
Am 07.11.2011 14:56, schrieb Paolo Bonzini: > On 11/07/2011 02:49 PM, Kevin Wolf wrote: >>> 2. eject with -f should really never be needed, but it does whatever is >>> needed to be able to follow up with a "change" command. It turns out it >>> is really "unlock" and "ask the guest to eject" combined, but that's the >>> implementation, not the model. >> >> Does this give different results than just asking the guest to eject >> without forcefully unlocking? I would expect that a guest that responds >> to the eject request would also unlock the drive. In which case I think >> eject without -f should be enough? > > Only if the guest is not buggy (e.g. locks the tray but stops polling > for eject requests) and has not crashed. If the guest is broken, forcefully unlocking and doing an eject request won't help either (the drive will be unlocked, but not ejected) and the only way to do anything useful with the drive is sending another eject command. This doesn't help with cleanly unmounting the medium, of course. So I think this is actually a point for the eject -f = paperclip model. Kevin
Paolo Bonzini <pbonzini@redhat.com> writes: > On 11/07/2011 02:21 PM, Markus Armbruster wrote: >> The commit message should explain why we need this callback. The cover >> letter says "support for eject requests is required by udev 173." >> Please elaborate on that. > > Well, first and foremost eject requests are in the spec. :) The fact > that recent guests need it is more a justification for pulling this in > 1.0. I agree we should try to solve the problem for 1.0. >> You implement it for IDE in PATCH 7/8 and SCSI in PATCH 8/8. You don't >> implement it for floppy, despite the comment. That's okay; floppy has >> no use for it. It's the comment that needs fixing. Devices that >> implement is_medium_locked() must implement this one as well. > > Right. > >> 1. eject without -f behaves like the physical tray button. It has >> immediate effect, unless the tray is locked closed. Then, the drive >> just notifies the OS of the button push, so the OS can react to it. The >> latter isn't implemented in QEMU. >> >> 2. eject with -f behaves like whatever physical way there is to pry the >> tray open, locked or not. CD-ROM drives commonly have a little button >> hidden in some hope you can reach with a bent paperclip. >> >> Could you explain your mental model? > > 1. eject without -f is as you mentioned. Would implementing the missing part help with the problem at hand? > 2. eject with -f should really never be needed, but it does whatever > is needed to be able to follow up with a "change" command. It turns > out it is really "unlock" and "ask the guest to eject" combined, but > that's the implementation, not the model. Physical hardware doesn't work that way (unless I misunderstand it). Always a warning sign. > The difference from the paperclip model is that it gives a chance for > the OS to clean up and eject safely. It wouldn't be hard to convince > me otherwise though, especially if it can help getting the patch in > 1.0. The "eject -f"+"change" can be replaced by "eject", possibly > followed by "eject -f" after a timeout, and then followed again by > "change". On bare metal, the pressing the tray button accomplishes that: the drive notifies the OS the button was pressed, the well-behaved OS cleans up and ejects. With a misbehaving OS, you're reduced to the paperclip. Can't we replicate that? 1. Tray button / eject without -f A. when the tray is not locked: tray opens immediately. B. when the tray is locked: OS gets notified. We expect it to clean up, unlock and open the tray at some point in the near future. 2. Paperclip / eject with -f Tray opens immediately. Guest OS may be unhappy about it.
On 11/07/2011 04:23 PM, Markus Armbruster wrote: >> 1. eject without -f is as you mentioned. > > Would implementing the missing part help with the problem at hand? It would help for non-buggy guests. Buggy means even "echo -1 > /sys/block/sr0/events_poll_msecs". However, a full solution would require a change in management, and adding a TRAY_STATUS_CHANGED event to QEMU. Not sure this is required for 1.0, as it can even be added later to stable. >> 2. eject with -f should really never be needed, but it does whatever >> is needed to be able to follow up with a "change" command. It turns >> out it is really "unlock" and "ask the guest to eject" combined, but >> that's the implementation, not the model. > > Physical hardware doesn't work that way (unless I misunderstand it). > Always a warning sign. True. >> The difference from the paperclip model is that it gives a chance for >> the OS to clean up and eject safely. It wouldn't be hard to convince >> me otherwise though, especially if it can help getting the patch in >> 1.0. The "eject -f"+"change" can be replaced by "eject", possibly >> followed by "eject -f" after a timeout, and then followed again by >> "change". > > On bare metal, the pressing the tray button accomplishes that: the drive > notifies the OS the button was pressed, the well-behaved OS cleans up > and ejects. With a misbehaving OS, you're reduced to the paperclip. > > Can't we replicate that? > > 1. Tray button / eject without -f > > A. when the tray is not locked: tray opens immediately. > > B. when the tray is locked: OS gets notified. We expect it to clean > up, unlock and open the tray at some point in the near future. > > 2. Paperclip / eject with -f > > Tray opens immediately. Guest OS may be unhappy about it. I now redid my tests with the paperclip behavior (it's really one line of different code) and everything seems to work. Will submit v2. Paolo
diff --git a/block.c b/block.c index 9873b57..53e21ba 100644 --- a/block.c +++ b/block.c @@ -821,6 +821,13 @@ bool bdrv_dev_has_removable_media(BlockDriverState *bs) return !bs->dev || (bs->dev_ops && bs->dev_ops->change_media_cb); } +void bdrv_dev_eject_request(BlockDriverState *bs, bool force) +{ + if (bs->dev_ops && bs->dev_ops->eject_request_cb) { + bs->dev_ops->eject_request_cb(bs->dev_opaque, force); + } +} + bool bdrv_dev_is_tray_open(BlockDriverState *bs) { if (bs->dev_ops && bs->dev_ops->is_tray_open) { diff --git a/block.h b/block.h index e77988e..d3c3d62 100644 --- a/block.h +++ b/block.h @@ -39,6 +39,12 @@ typedef struct BlockDevOps { */ void (*change_media_cb)(void *opaque, bool load); /* + * Runs when an eject request is issued from the monitor, the tray + * is closed, and the medium is locked. + * Device models with removable media must implement this callback. + */ + void (*eject_request_cb)(void *opaque, bool force); + /* * Is the virtual tray open? * Device models implement this only when the device has a tray. */ @@ -116,6 +122,7 @@ void bdrv_detach_dev(BlockDriverState *bs, void *dev); void *bdrv_get_attached_dev(BlockDriverState *bs); void bdrv_set_dev_ops(BlockDriverState *bs, const BlockDevOps *ops, void *opaque); +void bdrv_dev_eject_request(BlockDriverState *bs, bool force); bool bdrv_dev_has_removable_media(BlockDriverState *bs); bool bdrv_dev_is_tray_open(BlockDriverState *bs); bool bdrv_dev_is_medium_locked(BlockDriverState *bs); diff --git a/blockdev.c b/blockdev.c index 0827bf7..4cf333a 100644 --- a/blockdev.c +++ b/blockdev.c @@ -635,9 +635,11 @@ static int eject_device(Monitor *mon, BlockDriverState *bs, int force) qerror_report(QERR_DEVICE_NOT_REMOVABLE, bdrv_get_device_name(bs)); return -1; } - if (!force && !bdrv_dev_is_tray_open(bs) - && bdrv_dev_is_medium_locked(bs)) { - qerror_report(QERR_DEVICE_LOCKED, bdrv_get_device_name(bs)); + if (bdrv_dev_is_medium_locked(bs) && !bdrv_dev_is_tray_open(bs)) { + bdrv_dev_eject_request(bs, force); + if (!force) { + qerror_report(QERR_DEVICE_LOCKED, bdrv_get_device_name(bs)); + } return -1; } bdrv_close(bs);
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- block.c | 7 +++++++ block.h | 7 +++++++ blockdev.c | 8 +++++--- 3 files changed, 19 insertions(+), 3 deletions(-)