[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] vhost-user: fix watcher need be removed when vh
From: |
Marc-André Lureau |
Subject: |
Re: [Qemu-devel] [PATCH] vhost-user: fix watcher need be removed when vhost-user hotplug |
Date: |
Sun, 23 Jul 2017 12:06:29 +0200 |
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