Hi
On Fri, Sep 21, 2018 at 9:10 PM, Christophe Fergeau <cfergeau@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel