<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Fri, Dec 6, 2019 at 12:30 PM Frediano Ziglio <<a href="mailto:fziglio@redhat.com" target="_blank">fziglio@redhat.com</a>> wrote:<br><br>
> > By default command line devices are added using 'redirect-on-connect' filter,<br>
> > which do not fit the shard CD connecting requirements.<br><br>
> Can you describe the requirements here?<br></blockquote><div> </div><div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"></blockquote></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Fri, Dec 6, 2019 at 1:57 PM Yuri Benditovich <<a href="mailto:yuri.benditovich@daynix.com">yuri.benditovich@daynix.com</a>> wrote:<br>
My 2 cents:<br>
I think the goal here is to make something that the user _expects_.<br>
When he creates shared CD via command-line it does not expect it to<br>
behave exactly as already present disk-on-key - because for<br>
disk-on-key there are 2 posi sibilities: use it locally or redirect it<br>
immediately on connect to VM.<br>
With shared CD there is no option to use it locally so we try to<br>
redirect it automatically if this does not violate existing rules.<br></blockquote><div> </div><div><div>Sure, I will add a few lines on requirements to the commit message.</div><div>In general, I agree with Yuri's comments above, with some minor additions.</div></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
> This patch looks like an workaround to me.<br>
<br>
Emulated device created via command line is indeed a special kind of thing.<br>
>From one point of view it is a device that "exists" before connection.<br>
>From another point of view it is a device that was created especially<br>
to redirect it.<br>
Which solution you'd suggest?<br></blockquote><div> </div><div>I don't see any simple and elegant solution which suits the scope of this patch set. </div><div>Please propose, I will be happy to implement it, in place of this patch, or afterward as an improvement.</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> > diff --git a/src/usb-device-manager.c b/src/usb-device-manager.c<br></blockquote><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">> > @@ -901,10 +899,23 @@ static void<br>
> > spice_usb_device_manager_check_redir_on_connect(<br>
> > continue;<br>
> ><br>
> > bdev = spice_usb_device_manager_device_to_bdev(self, device);<br>
> > - if (spice_usb_backend_device_check_filter(<br>
> > - bdev,<br>
> > - priv->redirect_on_connect_rules,<br>
> > - priv->redirect_on_connect_rules_count) == 0) {<br>
> > + is_cd = spice_usb_backend_device_get_libdev(bdev) == NULL;<br>
><br>
> This is checking if emulated, not if is a CD.<br></blockquote><div> </div><div>I am changing this to call spice_usb_device_manager_is_device_shared_cd(), which logically correct</div><div>while its internal implementation may be subject to change, as discussed in other mail.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
><br>
> > +<br>
> > + if (priv->redirect_on_connect) {<br>
> > + shall_redirect = !spice_usb_backend_device_check_filter(<br>
> > + bdev,<br>
> > + priv->redirect_on_connect_rules,<br>
> > + priv->redirect_on_connect_rules_count);<br>
><br>
> Why not setting some "rules" and "rules_count" variable instead<br>
> and call spice_usb_backend_device_check_filter once?<br>
><br></blockquote><div>Which problem this is supposed to solve?</div><div> </div></div></div>