[Qemu-devel] [PATCH] block-backend: Always notify on blk_eject

[Qemu-devel] [PATCH] block-backend: Always notify on blk_eject

[John Snow]

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);
     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)
-- 
2.7.4

Re: [Qemu-devel] [PATCH] block-backend: Always notify on blk_eject

[John Snow]

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)
>

Re: [Qemu-devel] [PATCH] block-backend: Always notify on blk_eject

[Kevin Wolf]

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

Re: [Qemu-devel] [PATCH] block-backend: Always notify on blk_eject

[John Snow]

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
>