<div dir="ltr">Hi<div class="gmail_extra"><br><div class="gmail_quote">On Fri, Sep 21, 2018 at 9:10 PM, Christophe Fergeau <span dir="ltr"><<a href="mailto:cfergeau@redhat.com" target="_blank">cfergeau@redhat.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On Mon, Sep 17, 2018 at 04:22:50PM +0300, Yuri Benditovich wrote:<br>
> Changes from v3:<br>
> Single commit with all new files split to 12 patches<br>
> to make patch review easier<br>
<br>
</span>What you did is not splitting at all, adding one new file per commit,<br>
and then all the modifications to the existing code in a final commit<br>
is not what we meant when we asked for split patches.<br>
<br>
You asked why this is needed, and actually, it turns out your patches<br>
introduce a regression in virt-viewer even when not exercising at all<br>
any of the new code. The series as it is is not bisectable in any way,<br>
so I basically have to look at these 9000 lines of code to try to figure<br>
out what broke...<br></blockquote><div><br></div><div>IMO, the best thing to do in this case is to let me know about the regression</div><div>(there is no written test procedure AFAIK, so I could miss something in my tests)</div><div><br></div><div>Thanks,</div><div>Yuri</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
If the series was split, and if we'd start with some refactoring<br>
(reworking existing code without introducing any behaviour change), and then<br>
the introduction of the new features, then git would at least be able to<br>
tell me if it's the refactoring which introduced the bug, or the new<br>
feature. And I'd have less code to look at to figure out what's going<br>
on...<br>
<br>
I've started to do some splitting, see attached patch (win32 not tested<br>
at all, a quick usb redirection test on linux was working). I'd probably<br>
go further though, and remove isLibUsb for now, and maybe also the<br>
checks for LibusbContext being non-NULL. I did not do it for now as it<br>
would create lots of conflicts :)<br>
<br>
While on the topic of things which would cause conflicts, I'd really<br>
prefer if we did not use so much one and two letter variable names (ch,<br>
be, b, ..), this makes the code harder to read than it should.<br>
g_new0 will never return NULL, so you can remove the extra indentations<br>
in code like:<br>
foo = g_new0()<br>
if (foo) {<br>
}<br>
<br>
I would also add a spice_usb_backend_channel_<wbr>detach in addition to<br>
_attach. _attach is currently also doing the _detach, but it's making<br>
things much less readable. Same comment about spice_usb_backend_handle_<wbr>hotplug<br>
which could be split in enable_hotplug/disable_hotplug<br>
<br>
All your SPICE_DEBUG calls include a __FUNCTION__, but it seems<br>
SPICE_DEBUG already does that for you?<br>
<br>
#define SPICE_DEBUG(fmt, ...)                                   \<br>
    do {                                                        \<br>
        if (G_UNLIKELY(spice_util_get_<wbr>debug()))                 \<br>
            g_debug(G_STRLOC " " fmt, ## __VA_ARGS__);          \<br>
    } while (0)<br>
<br>
with<br>
<br>
#define G_STRLOC __FILE__ ":" G_STRINGIFY (__LINE__) ":" __PRETTY_FUNCTION__ "()"<br>
<br>
The "usbredirhost" stripping which is done in<br>
channel-usbredir.c:usbredir_<wbr>log should be moved to the equivalent<br>
function in usb-backend-common.c<br>
<span class="HOEnZb"><font color="#888888"><br>
Christophe<br>
</font></span></blockquote></div><br></div></div>