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