[Spice-devel] [spice-gtk v3 5/9] usb-redir: extend USB backend to support emulated devices

[Yuri Benditovich]

On Fri, Aug 23, 2019 at 10:59 AM Victor Toso <victortoso at redhat.com> wrote:
>
> On Thu, Aug 22, 2019 at 11:27:19AM +0300, Yuri Benditovich wrote:
> > On Thu, Aug 22, 2019 at 9:56 AM Frediano Ziglio <fziglio at redhat.com> wrote:
> > > > On Tue, Aug 13, 2019 at 2:59 PM Frediano Ziglio <fziglio at redhat.com> wrote:
> > > > > > +        ch->parser = ch->hidden_parser;
> > > > > > +        usbredirparser_do_write(ch->parser);
> > > > > >      }
> > > > > >  }
> > > > > >
> > > > > > @@ -807,12 +1347,16 @@ void
> > > > > > spice_usb_backend_channel_delete(SpiceUsbBackendChannel *ch)
> > > > > >      if (!ch) {
> > > > > >          return;
> > > > > >      }
> > > > > > -    if (ch->usbredirhost) {
> > > > > > -        usbredirhost_close(ch->usbredirhost);
> > > > > > +    if (ch->hidden_host) {
> > > > > > +        usbredirhost_close(ch->hidden_host);
> > > > > > +    }
>
> It was usbredirhost, now it is hidden_host /* Preparatory comment :) */
>
> > > > > > +    if (ch->hidden_parser) {
> > > > > > +        usbredirparser_destroy(ch->hidden_parser);
>
> A new thing, hidden_parser /* Preparatory comment :) */
>
> > > > > >      }
> > > > > >
> > > > > >      if (ch->rules) {
> > > > > > -        g_free(ch->rules);
> > > > > > +        /* rules were allocated by usbredirparser */
> > > > > > +        free(ch->rules);
>
> Rules no more g_free, now is free /* Preparatory comment :) */
>
> > > > >
> > > > > Minor :I suppose this small change can go to a preliminary patch too
> > > >
> > > > This code was here before, but it was never used as ch->rules was
> > > > never assigned.
> > >
> > > yes
> > >
> > > > Honestly I do not understand what this "preliminary patch"
> > > > should contain and what it is good for.
> > >
> > > I don't think is a good idea to add code with multiple logic
> > > for the same condition. Currently most of the code assume usb
> > > context is valid, I would go in a single direction, not
> > > adding additional code with 2 different login about it.
> >
> > I still do not understand what is 'preliminary patch', what it
> > should contain and why it is good to have it.
>
> If we say "preparatory patch" would make it clear for you? The
> goal is to review faster, that's what you want, I'm sure of it.
> Break it down as much as you can and we can check it and learn
> from it easier.

No, this does not make thing more clear for me.
It looks like in order to create some 'preparatory patch' I need to
introduce new fields that are not actually used and create some code
for them that will be changed in next patch.
Is the goal to make the review faster? The code already has been
reviewed several times in details.
I expect this 'preparatory patch' will make things more complicated
and will spawn additional rounds of discussions.

>
> To proper reply your questions so this will not extend:
>
> > what it should contain
>
> If code existed and you can change it before adding more code,
> that's what it should be the content.
>
> > why it is good to have it
>
> To have fewer discussions on possible regressions, too big
> patches and if a change is really needed or not - this should be
> valid enough reason.
>
> Amazes me a little that it takes 4 iterations, possible more yet,
> for a single line of review with the keywords "Minor" + "can go"
> + "preliminary patch".
>
> Could you please split or do you have an actual reason not to?

It is hard to do as I do not understand the goal.
I'd suggest you to make any 'preparatory patch' that you find useful.


>
> Btw, really happy with Frediano's review, many thanks!