<div dir="ltr"><div dir="ltr"><br></div><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Dec 6, 2019 at 12:04 PM Frediano Ziglio <<a href="mailto:fziglio@redhat.com">fziglio@redhat.com</a>> wrote:<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>> +gboolean<br>
> +spice_usb_device_manager_create_shared_cd_device(<br>
> +                                         SpiceUsbDeviceManager *self,<br>
> +                                         gchar                 *filename,<br>
> +                                         GError               **err)<br>
<br>
style, see <a href="https://www.spice-space.org/spice-project-coding-style-and-coding-conventions.html" rel="noreferrer" target="_blank">https://www.spice-space.org/spice-project-coding-style-and-coding-conventions.html</a>,<br>
here in a lot of declarations<br></blockquote><div>Do you mean argument alignment? Sure, to be fixed. </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">
Also for consistency in the other file "manager" is used, not "self".<br></blockquote><div>There is already no consistency. Some functions have been using 'self', some - 'manager'.</div><div>I suggest leaving it as is and producing a separate patch dedicated to this style issue so that we don't mix CD stuff with the global style fixes.</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>
It's weird that the create does not return the device so you cannot<br>
easily use spice_usb_device_manager_remove_shared_cd_device to remove it.<br></blockquote><div>Again, Yuri addressed this issue already in his mail.</div><div>It is a kind of design issue, as this is the way things are done right now.</div><div>I guess that it was implemented this way due to the asynchronous nature of the connecting process.</div><div>Anyway, it's a question of the flow management more than of a pure API definition.</div><div>Of course, I can imagine other types of design, but if necessary at all, such change should be done </div><div>across the board.</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">> +    bdev = spice_usb_device_manager_device_to_bdev(self, device);<br>
<br>
see below<br>
<br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">> +#ifdef USE_USBREDIR<br>
> +    SpiceUsbBackendDevice *bdev;<br>
> +    gboolean is_cd;<br>
> +<br>
> +    bdev = spice_usb_device_manager_device_to_bdev(self, device);<br>
<br>
Note that SpiceUsbBackendDevice is defined as<br>
<br>
    typedef struct _SpiceUsbDevice SpiceUsbBackendDevice;<br>
<br>
no need to call this function.<br>
<br></blockquote><div>I agree with Yuri. This is the current practice in all API functions.</div><div>We can rework it everywhere, or leave it as is.</div><div> <br class="gmail-Apple-interchange-newline"></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +    is_cd = (spice_usb_backend_device_get_libdev(bdev) == NULL) ? TRUE :<br>
> FALSE;<br>
<br>
just<br>
<br>
    is_cd = (spice_usb_backend_device_get_libdev(bdev) == NULL);<br></blockquote><div>OK </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>
Also the name of the function is wrong, you are not checking if the device<br>
is a shared cd but if is a emulated device.<br>
<br></blockquote><div>Well, I don't think the name spice_usb_device_manager_is_device_shared_cd is wrong. </div><div>It is an API function exposed to the external modules, and, just as the name suggests, </div><div>it promises to return True iff the device is a shared CD.</div><div>The implementation is based on the (currently true) fact, that only CDs have libdev==NULL.</div><div>It may seem awkward, but the alternative is to maintain an "is_cd" flag.</div><div>It is possible of course, but I opted for a minimal amount of changes in structures</div><div>and preferred to use existing functions even if their semantics may seem irrelevant :) </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">> @@ -1499,6 +1587,7 @@ gboolean spice_usb_device_is_isochronous(const<br>
> SpiceUsbDevice *info)<br>
>      return spice_usb_backend_device_isoch((SpiceUsbBackendDevice*) info);<br>
>  }<br>
>  <br>
> +<br>
<br>
spurious hunk, remove<br></blockquote><div>Of course!</div><div><br></div></div></div>