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 6E917A0C47; Tue, 23 Nov 2021 11:26:01 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id DA27940040; Tue, 23 Nov 2021 11:26:00 +0100 (CET) Received: from mail-wr1-f51.google.com (mail-wr1-f51.google.com [209.85.221.51]) by mails.dpdk.org (Postfix) with ESMTP id 942C04003C for ; Tue, 23 Nov 2021 11:25:59 +0100 (CET) Received: by mail-wr1-f51.google.com with SMTP id c4so38016254wrd.9 for ; Tue, 23 Nov 2021 02:25:59 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=e7zrZCKLmI9WRa29atJEJ5SD6LA42m1ufF2Xp8uQtAw=; b=XZLu7zdQIJNdYA0ra77HEx4nOu8v3h3TMtUHJaquh/wHXl2SJjDQBCIkHFCM9roKSm vAQXoh/bwk26Nr4JU+ePiXgdGbLFSiYL3y9Q3XbwxJkLqHUuYcnb2QoZrycsyEXHPpPL 303lD6giyQYYtW+UpwjzqUFB79RzDVup8T7yuAhQ3+mZj+XovtOIshw37ufKzjClGu4S ilPPgl2bWHTHcVOZuLFyN0M4bottCKt4laZozBrUA6QEY6nj4CsfyZCWg0ngqsDe//8/ puuuZbR2+cSwLXBNWT0TV0v1wtNFcWobYbNaRooQMDMIcmUf8V7290o2AqPC7iL4cW9H Vw9A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=e7zrZCKLmI9WRa29atJEJ5SD6LA42m1ufF2Xp8uQtAw=; b=xd5NevqNFjj/4W3r9RV/5bTc46zIKQITRVcAFOhMvqSsPc1dfdGJmbO3pXdLxSQvVW Dc73WUzINC+yokJIPanIwPx0/FGmIfKvmbgtqAIsZvFib7d7ZYQA7Omt418R3v66e05W 21XistahBUOAP2GocX2mEzHxcoaevQE9m/hBXF5N/o82PBmT+yGXvHlawswWuRXmpCpN HQQep78w8stpOxm8L3EKFIxXRCAtqsd//399YefgD3k57YLb628aX9B70f8Ido4aS/oI VMvSlKnEN3FFGyZPxMCsPs8TX89markt6HSzdTXfrgdNSXZrXmFc5wccLJBUm77ojCNl o/Rw== X-Gm-Message-State: AOAM530BeHN9k66h0bWsyG/ICuF9fgVdJM9A+tVsvGESYd/FD+tzX67m SQ9rG5dnvCXs1fOGPUWnx8uCuQ== X-Google-Smtp-Source: ABdhPJyXxre2iprt/4l7ato01ttwTdgcrrKxaWsqfaZMMxMXXUfeeu6e0OyPp0wkod5K687yhEFuew== X-Received: by 2002:a5d:668f:: with SMTP id l15mr6001525wru.182.1637663159269; Tue, 23 Nov 2021 02:25:59 -0800 (PST) Received: from 6wind.com ([2a01:e0a:5ac:6460:c065:401d:87eb:9b25]) by smtp.gmail.com with ESMTPSA id w4sm11603684wrs.88.2021.11.23.02.25.58 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 23 Nov 2021 02:25:58 -0800 (PST) Date: Tue, 23 Nov 2021 11:25:57 +0100 From: Olivier Matz To: Xueming Li Cc: dev@dpdk.org, Lior Margalit , Parav Pandit , Ray Kinsella , David Marchand Subject: Re: [PATCH] kvargs: fix device iterator match from arguments Message-ID: References: <20211122061250.3220823-1-xuemingl@nvidia.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20211122061250.3220823-1-xuemingl@nvidia.com> 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 Hi Xueming, On Mon, Nov 22, 2021 at 02:12:50PM +0800, Xueming Li wrote: > Device iterator RTE_DEV_FOREACH() failed to return devices from > classifier like "class=vdpa", because matching name from empty kvargs > returns no result. If device name not specified in kvargs, the function > should iterate all devices. > > This patch allows empty devargs or devargs without name specified. > > Fixes: 6aebb942907d ("kvargs: add function to get from key and value") > Cc: olivier.matz@6wind.com > > Signed-off-by: Xueming Li > --- > 21.11 specific bug, no copy to stable.org > --- > drivers/bus/auxiliary/auxiliary_params.c | 3 +++ > drivers/bus/vdev/vdev_params.c | 3 +++ > 2 files changed, 6 insertions(+) > > diff --git a/drivers/bus/auxiliary/auxiliary_params.c b/drivers/bus/auxiliary/auxiliary_params.c > index a9c7853ed1d..6a6382961ea 100644 > --- a/drivers/bus/auxiliary/auxiliary_params.c > +++ b/drivers/bus/auxiliary/auxiliary_params.c > @@ -27,6 +27,9 @@ auxiliary_dev_match(const struct rte_device *dev, > const struct rte_kvargs *kvlist = _kvlist; > const char *key = auxiliary_params_keys[RTE_AUXILIARY_PARAM_NAME]; > > + /* Iterate all devices if name not specified. */ > + if (kvlist == NULL || rte_kvargs_get(kvlist, key) == NULL) > + return 0; > if (rte_kvargs_get_with_value(kvlist, key, dev->name) == NULL) > return -1; > > diff --git a/drivers/bus/vdev/vdev_params.c b/drivers/bus/vdev/vdev_params.c > index 37d95395e7a..bab4c0d1d08 100644 > --- a/drivers/bus/vdev/vdev_params.c > +++ b/drivers/bus/vdev/vdev_params.c > @@ -29,6 +29,9 @@ vdev_dev_match(const struct rte_device *dev, > const struct rte_kvargs *kvlist = _kvlist; > const char *key = vdev_params_keys[RTE_VDEV_PARAM_NAME]; > > + /* Iterate all devices if name not specified. */ > + if (kvlist == NULL || rte_kvargs_get(kvlist, key) == NULL) > + return 0; > if (rte_kvargs_get_with_value(kvlist, key, dev->name) == NULL) > return -1; > Thank you for spotting and fixing this issue. The patch looks good to me, but may I suggest an alternative that would avoid to browse the kvlist twice? It is not yes tested, just for discussion. The idea is to add an errno for error cases of rte_kvargs_get_with_value() to identify the different cases. --- a/drivers/bus/auxiliary/auxiliary_params.c +++ b/drivers/bus/auxiliary/auxiliary_params.c @@ -27,7 +27,9 @@ auxiliary_dev_match(const struct rte_device *dev, const struct rte_kvargs *kvlist = _kvlist; const char *key = auxiliary_params_keys[RTE_AUXILIARY_PARAM_NAME]; - if (rte_kvargs_get_with_value(kvlist, key, dev->name) == NULL) + /* if kvlist is valid and contains the key, filter matching devices */ + if (rte_kvargs_get_with_value(kvlist, key, dev->name) == NULL && + rte_errno == ENOENT) return -1; return 0; diff --git a/drivers/bus/vdev/vdev_params.c b/drivers/bus/vdev/vdev_params.c index 37d95395e7..0a5a8a9f58 100644 --- a/drivers/bus/vdev/vdev_params.c +++ b/drivers/bus/vdev/vdev_params.c @@ -29,7 +29,9 @@ vdev_dev_match(const struct rte_device *dev, const struct rte_kvargs *kvlist = _kvlist; const char *key = vdev_params_keys[RTE_VDEV_PARAM_NAME]; - if (rte_kvargs_get_with_value(kvlist, key, dev->name) == NULL) + /* if kvlist is valid and contains the key, filter matching devices */ + if (rte_kvargs_get_with_value(kvlist, key, dev->name) == NULL && + rte_errno == ENOENT) return -1; return 0; diff --git a/lib/kvargs/rte_kvargs.c b/lib/kvargs/rte_kvargs.c index 11f624ef14..f1491715bf 100644 --- a/lib/kvargs/rte_kvargs.c +++ b/lib/kvargs/rte_kvargs.c @@ -209,17 +209,28 @@ const char * rte_kvargs_get_with_value(const struct rte_kvargs *kvlist, const char *key, const char *value) { + int key_found = 0; unsigned int i; - if (kvlist == NULL) + if (kvlist == NULL) { + rte_errno = EINVAL; return NULL; + } + for (i = 0; i < kvlist->count; ++i) { if (key != NULL && strcmp(kvlist->pairs[i].key, key) != 0) continue; + key_found = 1; if (value != NULL && strcmp(kvlist->pairs[i].value, value) != 0) continue; return kvlist->pairs[i].value; } + + if (key_found) + rte_errno = ENODEV; + else + rte_errno = ENOENT; + return NULL; } diff --git a/lib/kvargs/rte_kvargs.h b/lib/kvargs/rte_kvargs.h index 359a9f5b09..3cb22ece48 100644 --- a/lib/kvargs/rte_kvargs.h +++ b/lib/kvargs/rte_kvargs.h @@ -152,8 +152,12 @@ const char *rte_kvargs_get(const struct rte_kvargs *kvlist, const char *key); * The matching value. If NULL, any value will match. * * @return - * NULL if no key matches the input, - * a value associated with a matching key otherwise. + * The value associated with a matching key/value on success. + * On error, return NULL and rte_errno is set: + * - EINVAL - kvlist is NULL + * - ENOENT - no matching key/value tuple, but the key matches with + * a different value + * - ENODEV - key is not found in the kvlist */ __rte_experimental const char *rte_kvargs_get_with_value(const struct rte_kvargs *kvlist, Let me know if it would work for you. I can submit a patch if you want. I can add a unit test for kvargs, but do you know where we could add a unit test for the dev iterate? Thanks, Olivier