Re: [Qemu-devel] [PATCH] vhost-user: fix watcher need be removed when vhost-user hotplug

[Marc-André Lureau]

Hi

On Sun, Jul 23, 2017 at 4:12 AM, Michael S. Tsirkin <address@hidden> wrote:
> On Sat, Jul 22, 2017 at 09:24:27AM +0000, Marc-André Lureau wrote:
>>
>>
>> On Sat, Jul 22, 2017 at 2:35 AM Michael S. Tsirkin <address@hidden> wrote:
>>
>>     On Fri, Jul 21, 2017 at 11:19:04AM +0000, Marc-André Lureau wrote:
>>     > Hi
>>     >
>>     > On Fri, Jul 21, 2017 at 7:18 AM w00273186 <address@hidden> wrote:
>>     >
>>     >     From: Yunjian Wang <address@hidden>
>>     >
>>     >     "nc" is freed after hotplug vhost-user, but the watcher don't be
>>     removed.
>>     >     The QEMU crash when the watcher access the "nc" on socket 
>> disconnect.
>>     >
>>     >
>>     >
>>     > This is actually your 3rd iteration on the patch
>>     >
>>     > Could your describe your changes since:
>>     > "[PATCH v2] vhost-user: fix watcher need be removed when vhost-user
>>     hotplug"
>>     >
>>     > Thanks
>>
>>     Yes but it's a 3-liner. That's way below the limit where you need
>>     detailed change history. Does the patch make sense to you?
>>
>>
>>
>> That's not all, the fact that he didn't come up with the same solution in the
>> first place, and I didn't notice a problem either with the previous approach 
>> is
>> enough to ask from some clarification on which approach is best, and I bet
>> there is something to say.
>
> I'm rather confused.  Looks like you were the one who asked for the change.
> Really we want to attract new contributors and a small bugfix like this
> seems like a very good way to start contributing. Changelog is already
> 3 times the size of the patch here. So I think we should just get the patch
> reviewed and applied if correct. Do you plan to review it?

Indeed, but I totally forgot.

This situation wouldn't happen if:
- the patch was version v3
- the patch/mail would have been annotated after  --- to quickly
describe the change
- I had better memory...


>
>> Furthermore, we would really benefit from having repeatable cases for this 
>> kind
>> of fixes.
>
> I agree disconnect path is but tested adequately but I don't think we
> are at a point where we should be asking for testcases for every use
> after free bug that gets fixed.

Not to write a test case, but at least to document what triggered this
path. Since Yunjian gave it in the previous reply, and I forgot that
too, it would be best to have it in the commit message, agree?




-- 
Marc-André Lureau