| Message ID | 1477689838-27053-1-git-send-email-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 10/28/2016 05:23 PM, John Snow wrote: > blk_eject is only used by scsi-disk and atapi, and in both cases we > only attempt to invoke blk_eject if we have a bona-fide change in > tray state. > > The "issue" here is that the tray state does not generate a QMP event > unless there is a medium/BDS attached to the device, so if libvirt et al > are waiting for a tray event to occur from an empty-but-closed drive, > software opening that drive will not emit an event and libvirt will > wait forever. > > Change this by modifying blk_eject to always emit an event, instead of > conditionally on a "real" backend eject. > > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1373264 > > Reported-by: Peter Krempa <pkrempa@redhat.com> > Signed-off-by: John Snow <jsnow@redhat.com> > --- > block/block-backend.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/block/block-backend.c b/block/block-backend.c > index c53ca30..2044f20 100644 > --- a/block/block-backend.c > +++ b/block/block-backend.c > @@ -1381,15 +1381,16 @@ void blk_eject(BlockBackend *blk, bool eject_flag) > /* blk_eject is only called by qdevified devices */ > assert(!blk->legacy_dev); > > + id = blk_get_attached_dev_id(blk); Uhh, whoops, I left a change in my local branch that kept this right before the tray event. Sorry about that. > if (bs) { > bdrv_eject(bs, eject_flag); > - > - id = blk_get_attached_dev_id(blk); > - qapi_event_send_device_tray_moved(blk_name(blk), id, > - eject_flag, &error_abort); > - g_free(id); > - > } > + > + /* Whether or not we ejected on the backend, > + * the frontend experienced a tray event. */ > + qapi_event_send_device_tray_moved(blk_name(blk), id, > + eject_flag, &error_abort); > + g_free(id); > } > > int blk_get_flags(BlockBackend *blk) >
Am 28.10.2016 um 23:30 hat John Snow geschrieben: > On 10/28/2016 05:23 PM, John Snow wrote: > >blk_eject is only used by scsi-disk and atapi, and in both cases we > >only attempt to invoke blk_eject if we have a bona-fide change in > >tray state. > > > >The "issue" here is that the tray state does not generate a QMP event > >unless there is a medium/BDS attached to the device, so if libvirt et al > >are waiting for a tray event to occur from an empty-but-closed drive, > >software opening that drive will not emit an event and libvirt will > >wait forever. > > > >Change this by modifying blk_eject to always emit an event, instead of > >conditionally on a "real" backend eject. > > > >Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1373264 > > > >Reported-by: Peter Krempa <pkrempa@redhat.com> > >Signed-off-by: John Snow <jsnow@redhat.com> > >--- > > block/block-backend.c | 13 +++++++------ > > 1 file changed, 7 insertions(+), 6 deletions(-) > > > >diff --git a/block/block-backend.c b/block/block-backend.c > >index c53ca30..2044f20 100644 > >--- a/block/block-backend.c > >+++ b/block/block-backend.c > >@@ -1381,15 +1381,16 @@ void blk_eject(BlockBackend *blk, bool eject_flag) > > /* blk_eject is only called by qdevified devices */ > > assert(!blk->legacy_dev); > > > >+ id = blk_get_attached_dev_id(blk); > > Uhh, whoops, I left a change in my local branch that kept this right > before the tray event. Sorry about that. The patch looks good to me, but moving the line would indeed be nicer. Are you going to send a v2 then? Also, at the risk of sounding like a broken record, I think a test case would be nice. Kevin
On 10/31/2016 10:13 AM, Kevin Wolf wrote: > Am 28.10.2016 um 23:30 hat John Snow geschrieben: >> On 10/28/2016 05:23 PM, John Snow wrote: >>> blk_eject is only used by scsi-disk and atapi, and in both cases we >>> only attempt to invoke blk_eject if we have a bona-fide change in >>> tray state. >>> >>> The "issue" here is that the tray state does not generate a QMP event >>> unless there is a medium/BDS attached to the device, so if libvirt et al >>> are waiting for a tray event to occur from an empty-but-closed drive, >>> software opening that drive will not emit an event and libvirt will >>> wait forever. >>> >>> Change this by modifying blk_eject to always emit an event, instead of >>> conditionally on a "real" backend eject. >>> >>> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1373264 >>> >>> Reported-by: Peter Krempa <pkrempa@redhat.com> >>> Signed-off-by: John Snow <jsnow@redhat.com> >>> --- >>> block/block-backend.c | 13 +++++++------ >>> 1 file changed, 7 insertions(+), 6 deletions(-) >>> >>> diff --git a/block/block-backend.c b/block/block-backend.c >>> index c53ca30..2044f20 100644 >>> --- a/block/block-backend.c >>> +++ b/block/block-backend.c >>> @@ -1381,15 +1381,16 @@ void blk_eject(BlockBackend *blk, bool eject_flag) >>> /* blk_eject is only called by qdevified devices */ >>> assert(!blk->legacy_dev); >>> >>> + id = blk_get_attached_dev_id(blk); >> >> Uhh, whoops, I left a change in my local branch that kept this right >> before the tray event. Sorry about that. > > The patch looks good to me, but moving the line would indeed be nicer. > Are you going to send a v2 then? > Yep. > Also, at the risk of sounding like a broken record, I think a test case > would be nice. > "What's good for the goose is good for the gander" I'll send tests for these two patches. --js > Kevin >
diff --git a/block/block-backend.c b/block/block-backend.c index c53ca30..2044f20 100644 --- a/block/block-backend.c +++ b/block/block-backend.c @@ -1381,15 +1381,16 @@ void blk_eject(BlockBackend *blk, bool eject_flag) /* blk_eject is only called by qdevified devices */ assert(!blk->legacy_dev); + id = blk_get_attached_dev_id(blk); if (bs) { bdrv_eject(bs, eject_flag); - - id = blk_get_attached_dev_id(blk); - qapi_event_send_device_tray_moved(blk_name(blk), id, - eject_flag, &error_abort); - g_free(id); - } + + /* Whether or not we ejected on the backend, + * the frontend experienced a tray event. */ + qapi_event_send_device_tray_moved(blk_name(blk), id, + eject_flag, &error_abort); + g_free(id); } int blk_get_flags(BlockBackend *blk)
blk_eject is only used by scsi-disk and atapi, and in both cases we only attempt to invoke blk_eject if we have a bona-fide change in tray state. The "issue" here is that the tray state does not generate a QMP event unless there is a medium/BDS attached to the device, so if libvirt et al are waiting for a tray event to occur from an empty-but-closed drive, software opening that drive will not emit an event and libvirt will wait forever. Change this by modifying blk_eject to always emit an event, instead of conditionally on a "real" backend eject. Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1373264 Reported-by: Peter Krempa <pkrempa@redhat.com> Signed-off-by: John Snow <jsnow@redhat.com> --- block/block-backend.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)