Re: [PATCH 07/18] vhost-vdpa: tweak the error label in vhost_vdpa_add()

[Stefano Garzarella <sgarzare@redhat.com>]

On Tue, Jul 06, 2021 at 04:10:22PM +0800, Jason Wang wrote:
>
>在 2021/7/6 下午4:03, Jason Wang 写道:
>>
>>在 2021/6/23 下午11:03, Stefano Garzarella 写道:
>>>On Mon, Jun 21, 2021 at 12:16:39PM +0800, Jason Wang wrote:
>>>>Introduce new error label to avoid the unnecessary checking of net
>>>>pointer.
>>>>
>>>>Fixes: 1e0a84ea49b68 ("vhost-vdpa: introduce vhost-vdpa net client")
>>>>Signed-off-by: Jason Wang <jasowang@redhat.com>
>>>>---
>>>>net/vhost-vdpa.c | 13 ++++++-------
>>>>1 file changed, 6 insertions(+), 7 deletions(-)
>>>>
>>>>diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
>>>>index 21f09c546f..0da7bc347a 100644
>>>>--- a/net/vhost-vdpa.c
>>>>+++ b/net/vhost-vdpa.c
>>>>@@ -100,19 +100,18 @@ static int vhost_vdpa_add(NetClientState 
>>>>*ncs, void *be)
>>>>    net = vhost_net_init(&options);
>>>>    if (!net) {
>>>>        error_report("failed to init vhost_net for queue");
>>>>-        goto err;
>>>>+        goto err_init;
>>>>    }
>>>>    s->vhost_net = net;
>>>>    ret = vhost_vdpa_net_check_device_id(net);
>>>>    if (ret) {
>>>>-        goto err;
>>>>+        goto err_check;
>>>>    }
>>>>    return 0;
>>>>-err:
>>>>-    if (net) {
>>>>-        vhost_net_cleanup(net);
>>>>-        g_free(net);
>>>>-    }
>>>>+err_check:
>>>>+    vhost_net_cleanup(net);
>>>>+    g_free(net);
>>>
>>>Should we set s->vhost_net to NULL to avoid use after free?
>>>
>>>Then we should also remove the `assert(s->vhost_net)` in 
>>>net_vhost_vdpa_init() since we can fail.
>>
>>
>>Right, will do this in a separate patch.
>
>
>I just forget the job has been done in the next patch :)

I saw it later too ;-)

>
>So we are fine here.

Yep for the assert(), but what about setting s->vhost_net to NULL?
Or just move the s->vhost_net assignment just before the `return 0`.

Thanks,
Stefano