From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 01DD1A052A; Mon, 25 Jan 2021 17:16:38 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 7F985140FCE; Mon, 25 Jan 2021 17:16:38 +0100 (CET) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by mails.dpdk.org (Postfix) with ESMTP id 22B98140FBD for ; Mon, 25 Jan 2021 17:16:36 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1611591396; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=p0DxiEbiujdjENVCAp0TptpT8TsN0t6UAdhxlRzDc/w=; b=E5GA4HBuiuxj1vta4WtGc6nlJrHK+03YlHFtqrg9EXKWfArtGAlVe1GQMqZcLuhxVk+PXy 3hgPuEa62jzAtxC1I8qntRZP6N4WiMVhGca3VlUrKtOhK85n9SXX4uivxcBx+a3bLgkSPM LfTO2ecMiPx11JnADlQitTMcr/3l0i0= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-185-DcUMJ2fQNg2xReVhlX1jOQ-1; Mon, 25 Jan 2021 11:16:31 -0500 X-MC-Unique: DcUMJ2fQNg2xReVhlX1jOQ-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 7054A9CC1E; Mon, 25 Jan 2021 16:16:30 +0000 (UTC) Received: from [10.36.110.31] (unknown [10.36.110.31]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 3777F60C47; Mon, 25 Jan 2021 16:16:26 +0000 (UTC) To: "Xia, Chenbo" , "dev@dpdk.org" , "olivier.matz@6wind.com" , "amorenoz@redhat.com" , "david.marchand@redhat.com" References: <20210119212507.1043636-1-maxime.coquelin@redhat.com> <20210119212507.1043636-45-maxime.coquelin@redhat.com> From: Maxime Coquelin Message-ID: <5a90a987-bf07-7b8a-4320-772e4d8197f7@redhat.com> Date: Mon, 25 Jan 2021 17:16:24 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=maxime.coquelin@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v2 44/44] net/virtio: handle Virtio-user setup failure properly X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 1/22/21 10:24 AM, Xia, Chenbo wrote: > Hi Maxime, > >> -----Original Message----- >> From: Maxime Coquelin >> Sent: Wednesday, January 20, 2021 5:25 AM >> To: dev@dpdk.org; Xia, Chenbo ; olivier.matz@6wind.com; >> amorenoz@redhat.com; david.marchand@redhat.com >> Cc: Maxime Coquelin >> Subject: [PATCH v2 44/44] net/virtio: handle Virtio-user setup failure >> properly >> >> This patch fixes virtio_user_dev_setup() error path, >> by cleaning all resources it allocates. >> >> Suggested-by: Adrian Moreno >> Signed-off-by: Maxime Coquelin >> --- >> drivers/net/virtio/virtio_user/virtio_user_dev.c | 11 +++++++++-- >> 1 file changed, 9 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c >> b/drivers/net/virtio/virtio_user/virtio_user_dev.c >> index a1e32158bb..2f7dc574b6 100644 >> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c >> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c >> @@ -427,15 +427,22 @@ virtio_user_dev_setup(struct virtio_user_dev *dev) >> >> if (virtio_user_dev_init_notify(dev) < 0) { >> PMD_INIT_LOG(ERR, "(%s) Failed to init notifiers\n", dev->path); >> - return -1; >> + goto destroy; >> } >> >> if (virtio_user_fill_intr_handle(dev) < 0) { >> PMD_INIT_LOG(ERR, "(%s) Failed to init interrupt handler\n", dev- >>> path); >> - return -1; >> + goto uninit; >> } >> >> return 0; >> + >> +uninit: >> + virtio_user_dev_uninit(dev); > > IMHO, it may not be the best choice to call virtio_user_dev_uninit here.. > > Logically when virtio_user_fill_intr_handle fails, we want to release the resources > which is allocated in virtio_user_dev_init_notify (i.e., fds), but virtio_user_dev_uninit > does more. For example, unregister the event callback that have not been registered yet and > it also leads to destroy callback called twice. Although things done but not needed will > not lead to error, but it looks not in the best way. What do you think? I agree, I'm reworking it for v3. Kick and call FDs will be initialized to -1 at virtio_user_dev_init() time. I introduce a virtio_user_dev_uninit_notify that will close all kick and call FDs is opened and reset their value to -1. Thenb I call this function in the error path of virtio_user_dev_setup() and also in virtio_user_dev_uninit(). Doing that, I can also simplifies virtio_user_dev_init_notify() and only loop for max queue pairs instead of VIRTIO_MAX_VIRTQUEUES and so avoid setting FDs to -1 a second time. Thanks, Maxime > Thanks, > Chenbo > >> +destroy: >> + dev->ops->destroy(dev); >> + >> + return -1; >> } >> >> /* Use below macro to filter features from vhost backend */ >> -- >> 2.29.2 >