From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from out3-smtp.messagingengine.com (out3-smtp.messagingengine.com [66.111.4.27]) by dpdk.org (Postfix) with ESMTP id 074595F16 for ; Mon, 5 Nov 2018 10:46:42 +0100 (CET) Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.nyi.internal (Postfix) with ESMTP id 944BF20E0E; Mon, 5 Nov 2018 04:46:41 -0500 (EST) Received: from mailfrontend1 ([10.202.2.162]) by compute1.internal (MEProxy); Mon, 05 Nov 2018 04:46:41 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding:content-type; s=mesmtp; bh=7VKmZqVaVdI1HrQBioBLx6Aq8j06+CUe4IM8l9lFOQ4=; b=cCnysEMffmNO S8+qmOmm8iv7gAWzk1oRsFZEQp1Aa+VkOUo4u8uWaDRNK1uirMKoqSgjAxEdb8PG PXLzpR0jbXYr7EK3PPxWOCdgAShPCYKYDykx+HJKVQpNAHvldjyiUET3Otlgrnkf 7lOPNG/A1H5fXifqykV5bFRPnBpZoho= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender :x-sasl-enc; s=fm1; bh=7VKmZqVaVdI1HrQBioBLx6Aq8j06+CUe4IM8l9lFO Q4=; b=W4evMhd4DH6BF1f958GCugmJMS62b+eQXOgR6nkWPm/uVWqc+LoQGIW2/ 14nF4nYq4wj85Z56MvJ/pst3IcYkpu0mCcqgBUSFI4WVY9nzwdYcTpcnxy3s9RqL sWT1ga3/Bbv4wwqq7RXnpM+chSnUbrI5h93B39OynOIEpScRYQ1cp62GZtPf4muw oQc64s3O3qpMs08Ulx16o1zV8iDJ+0Zt9DUQCi/GU2YRnu1rmT0x/eFZGz3pn+6W v681DQOZ68uu6TwNLOSlerO0/S/qbeU6MOEl3YAZAyfJs5lcf9bnP2hcf1TfpBKO 0egl2rgn642Zj3hQw/hjTVOcBPMYw== X-ME-Sender: X-ME-Proxy: Received: from xps.localnet (184.203.134.77.rev.sfr.net [77.134.203.184]) by mail.messagingengine.com (Postfix) with ESMTPA id 4A4A3E48B9; Mon, 5 Nov 2018 04:46:40 -0500 (EST) From: Thomas Monjalon To: "Stojaczyk, Dariusz" Cc: "dev@dpdk.org" , gaetan.rivet@6wind.com Date: Mon, 05 Nov 2018 10:46:39 +0100 Message-ID: <4918273.sFLXtBqTXr@xps> In-Reply-To: References: <20181105070447.67700-1-dariusz.stojaczyk@intel.com> <4545148.O9CyC7tLm6@xps> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Subject: Re: [dpdk-dev] [PATCH 2/3] devargs: delay freeing previous devargs when overriding them X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 05 Nov 2018 09:46:42 -0000 05/11/2018 09:25, Stojaczyk, Dariusz: > Hi Thomas, > > From: Thomas Monjalon [mailto:thomas@monjalon.net] > > 05/11/2018 08:04, Darek Stojaczyk: > > > -int __rte_experimental > > > -rte_devargs_insert(struct rte_devargs *da) > > > +void __rte_experimental > > > +rte_devargs_insert(struct rte_devargs *da, struct rte_devargs > > **prev_da) > > > > You should update the API section of the release notes. > > Even for experimental API? OK, I didn't know it's needed. Yes, even for experimental API, the API changes must documented. > > > { > > > - int ret; > > > + struct rte_devargs *d; > > > + void *tmp; > > > + > > > + *prev_da = NULL; > > > + TAILQ_FOREACH_SAFE(d, &devargs_list, next, tmp) { > > > + if (strcmp(d->bus->name, da->bus->name) == 0 && > > > + strcmp(d->name, da->name) == 0) { > > > + TAILQ_REMOVE(&devargs_list, d, next); > > > + *prev_da = d; > > > + break; > > > + } > > > + } > > > > > > - ret = rte_devargs_remove(da); > > > - if (ret < 0) > > > - return ret; > > > > Why not updating rte_devargs_remove instead of duplicating its code? > > We still want to preserve the functionality of rte_devargs_remove. > rte_devargs_remove does TAILQ_REMOVE + free; rte_devargs_insert does just TAILQ_REMOVE. (I think I also forgot to update rte_devargs_insert documentation, I'll do that in V2) Yes, because of the rollback, OK. Please mention in devargs_insert doc that the old devargs can be used for rollback. > Since you've mentioned it: > Eventually I'd see rte_devargs_remove to accept the exact same devargs parameter that was passed to rte_devargs_insert. Then rte_devargs_remove wouldn't do any sort of lookup. Maybe additional rte_devargs_find(const char *name) could be added for existing cases where the original devargs struct is not available. However, I'm not familiar enough with this code to perform the refactor and am just trying to fix the stuff. Still, how does it sound? I think we can keep it as is. We can re-think the whole thing in the next release. I think we should not play with devargs list as we do. There should be only lists for scanned devices of each bus and that's all.