On Fri, Aug 12, 2022 at 10:50 PM Victor Toso <victort...@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 <victort...@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 >