Re: [PATCH 3/3] usbredir: avoid queuing hello packet on snapshot restore

[Joelle van Dyne]

On Fri, Aug 12, 2022 at 10:50 PM Victor Toso <victortoso@redhat.com> wrote:
>
> Hi,
>
> On Fri, Aug 12, 2022 at 10:33:54PM -0700, Joelle van Dyne wrote:
> > On Fri, Aug 12, 2022 at 10:30 PM Victor Toso <victortoso@redhat.com> wrote:
> > >
> > > Hi,
> > >
> > > On Fri, Aug 12, 2022 at 06:10:31PM -0700, Joelle van Dyne wrote:
> > > > When launching QEMU with "-loadvm", usbredir_create_parser() should 
> > > > avoid
> > > > setting up the hello packet (just as with "-incoming". On the latest 
> > > > version
> > > > of libusbredir, usbredirparser_unserialize() will return error if the 
> > > > parser
> > > > is not "pristine."
> > >
> > > That was wrong in the usbredir side. The fix [0] was merged and
> > > included in the latest 0.13.0 release
> >
> > This is good to know. Should the entire runstate_check in
> > usbredir_create_parser be removed?
>
> From my POV your patch looks correct and would avoid migration
> issues such as [1] with usbredir 0.12.0 that was not patched
>
> [1] https://bugzilla.redhat.com/show_bug.cgi?id=2096008
>
> So, I'd keep the check and the patch :)

I have to admit I'm not too familiar with the inner workings of
libusbredir. But is it desirable to skip the HELLO packet on "loadvm"?
I wrote this on the assumption that it's correct because libusbredir
enforces it, but if that's wrong, then I'm wondering if maybe we need
the HELLO to re-establish communication (that was the issue I triaged
with "-S", when USB devices did not work due to the HELLO packet not
being sent). In a migration, it makes sense that a SPICE client has
not reset the USB device. However, when re-starting QEMU with
"-loadvm", it's possible the USB device has been disconnected and
reconnected. Ideally, we report that to the guest and let it handle
the reset. Would "usbredirparser_fl_no_hello" prevent that?

>
> > > [0] https://gitlab.freedesktop.org/spice/usbredir/-/merge_requests/61
> > >
> > > > Signed-off-by: Joelle van Dyne <j@getutm.app>
> > > > ---
> > > >  hw/usb/redirect.c | 3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
> > > > index fd7df599bc..47fac3895a 100644
> > > > --- a/hw/usb/redirect.c
> > > > +++ b/hw/usb/redirect.c
> > > > @@ -1280,7 +1280,8 @@ static void usbredir_create_parser(USBRedirDevice 
> > > > *dev)
> > > >      }
> > > >  #endif
> > > >
> > > > -    if (runstate_check(RUN_STATE_INMIGRATE)) {
> > > > +    if (runstate_check(RUN_STATE_INMIGRATE) ||
> > > > +        runstate_check(RUN_STATE_RESTORE_VM)) {
> > > >          flags |= usbredirparser_fl_no_hello;
> > > >      }
> > > >      usbredirparser_init(dev->parser, VERSION, caps, 
> > > > USB_REDIR_CAPS_SIZE,
> > > > --
> > > > 2.28.0
> > > >
> > > >
>
> Cheers,
> Victor
>