Hi On Sun, Jul 23, 2017 at 4:12 AM, Michael S. Tsirkin <m...@redhat.com> 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 <m...@redhat.com> 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 <wangyunj...@huawei.com> >> wrote: >> > >> > From: Yunjian Wang <wangyunj...@huawei.com> >> > >> > "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
