[Spice-devel] [spice-gtk v4 00/13] CD sharing feature

[Yuri Benditovich]

On Sat, Sep 22, 2018 at 12:14 AM, Yuri Benditovich <
yuri.benditovich at daynix.com> wrote:

> Hi
>
> On Fri, Sep 21, 2018 at 9:10 PM, Christophe Fergeau <cfergeau at 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...
>>
>
BTW I'm sorry to write it, but for the record the patch is not 9000 lines,
if is less than 2500 lines including all.
I believe if you describe the kind of regression I am able to find root
cause
of it quickly.

Thanks,
Yuri



>
> 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
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20180922/e7436587/attachment.html>