On (Thu) 16 Jul 2015 [10:11:04], Nils Carlson wrote:
> On Thu, 16 Jul 2015, Amit Shah wrote:
>
> >On (Wed) 15 Jul 2015 [23:44:52], Nils Carlson wrote:
> >>On Mon, 13 Jul 2015, Nils Carlson wrote:
> >>
> >>>On Mon, 13 Jul 2015, Amit Shah wrote:
> >>
> >><snip>
> >>
> >>>>Also, returning TRUE there isn't right - if the connection ends, we
> >>>>should return FALSE.
> >>>
> >>>I agree that this seems reasonable. I will change it and re-test.
> >>>
> >>
> >>I had a closer look, and it seems always returning true is intentional here,
> >>the called function, tcp_chr_disconnect(chr), handles the deregistration
> >>from handlers. If we were to return FALSE we would be duplicating work and
> >>possibly breaking things.
> >
> >Not sure how.
> >
> >Anyway, can you please start a new thread, with the author and
> >reviewers of the patch CC'ed, so they can chime in as well?
>
> The authors and reviewers of which patch exactly?
>
> The fix for windows which broke cloudstack was done in 812c1057.
Yea, the author of this patch. Also, I meant the return TRUE in this
patch, not the one Paolo added.
> The returning TRUE everywhere was done in commit cdbf6e16 by Paolo Bonzini,
> CC:d above.
>
> I don't think there was anything wrong with either patch, the former one
> simply broken a strange behaviour CloudStack was relying on which I would
> say is dubious (fire and forget messaging.)
There need not be anything wrong with a previous patch; but just
because someone touched the area recently, they might have more
context, and even help in spotting the bug.
Amit
