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

[Victor Toso]

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.

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?

Btw, really happy with Frediano's review, many thanks!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20190823/58fbed9e/attachment.sig>