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@xxxxxxxxxx> wrote:
> > > On Tue, Aug 13, 2019 at 2:59 PM Frediano Ziglio <fziglio@xxxxxxxxxx> 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!
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel