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 DF7CBA0C52; Wed, 24 Nov 2021 13:14:58 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id CC47141157; Wed, 24 Nov 2021 13:14:58 +0100 (CET) Received: from mail-wr1-f54.google.com (mail-wr1-f54.google.com [209.85.221.54]) by mails.dpdk.org (Postfix) with ESMTP id 36B1240F35 for ; Wed, 24 Nov 2021 13:14:58 +0100 (CET) Received: by mail-wr1-f54.google.com with SMTP id u18so3817922wrg.5 for ; Wed, 24 Nov 2021 04:14:58 -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=/6ic0OqaVqm74d9d1bhVWdIGsPam19cutBielgoEhdI=; b=MruM5MCy6bW4oWlSTzngjqzDxD7MhhOHBU1xrHvjVZds7a2+Yy1sqyDhwE33rftyJH e/Hx3ovS0ISUFD5iFi4qBRXxOSX2EnfsGggpaY7RM5X6diCvDIF6a5Lr1axLadlCn9vR QWAJNxtNKRbgKHOPI5/Kn1atYuIBD2ij2N4S8iBSjVHowTC3X0hedwPMLji/HaDZeDZ5 hd9QHl8Ei3FjpKuzntg7XzRG8cyY8y8dktMyDD7l4AUY6nH4/a9hBeMo4azmLKsRkCBn 5KaaDMuUvCOT/GlWZ065PUzxrsK3257YOTgGBIU3F/c75Nft5Rlcnqx+jfoVJx9nNh8H Xf6g== 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=/6ic0OqaVqm74d9d1bhVWdIGsPam19cutBielgoEhdI=; b=3eFH9u41N9iBFSI6h51dt26ThSKTV6w4n/TO2w5lgz5HB9MFz2iBmo6pwtft8yFf6O iPMRxQ70Vga1DMOwCQm+244LwBxWFTRO9eBjxqEWd67c7vfrUM6q6ODSH/50ftxCl2nc KOk2gO4LKJ8mB2vuRB3KT/PrbcAp5hkpUkMzbvslBLRouLVXeJEITf4bIEHKxTnMlalb 2MCeTX3qOCiPiEslAsCn2nmAx9kOUP5ZteA8wkYORX70LM0pOdHBcs4N8/zosk6/Vk7J g9scsmWXs2gv4ekomtxtfogtNtMzrNvyfmzJfYEOlSaTnjMF0kSfOxBSo7Fiyw5tOPgp v4UA== X-Gm-Message-State: AOAM530B2jDKuQHmm+wbVpz90q0ONP8/KVx5M5dSGX/98OPjUzh86bdq J0qfqm6ZwJyIzb/W1Jxj1pEROQ== X-Google-Smtp-Source: ABdhPJzRn8iuYv61LJUnr8IYoP4zS5NN0/uzU3OPOZLotyXzUeYIVRF0S6jdkJHk1ga3jBRPmiZrOg== X-Received: by 2002:a5d:42cc:: with SMTP id t12mr18011587wrr.129.1637756097893; Wed, 24 Nov 2021 04:14:57 -0800 (PST) Received: from 6wind.com ([2a01:e0a:5ac:6460:c065:401d:87eb:9b25]) by smtp.gmail.com with ESMTPSA id p27sm4286456wmi.28.2021.11.24.04.14.56 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 24 Nov 2021 04:14:57 -0800 (PST) Date: Wed, 24 Nov 2021 13:14:56 +0100 From: Olivier Matz To: "Xueming(Steven) Li" Cc: "dev@dpdk.org" , Lior Margalit , Parav Pandit , "david.marchand@redhat.com" , "mdr@ashroe.eu" Subject: Re: [PATCH v3] bus: fix device iterator match from arguments Message-ID: References: <20211122061250.3220823-1-xuemingl@nvidia.com> <20211124110244.26464-1-olivier.matz@6wind.com> <7f51c5b49ea14c8f50fb73bfb30a2b377b8f7488.camel@nvidia.com> <858f3c9302ee4810d8552defa99a33d9820b0e11.camel@nvidia.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <858f3c9302ee4810d8552defa99a33d9820b0e11.camel@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 On Wed, Nov 24, 2021 at 11:43:29AM +0000, Xueming(Steven) Li wrote: > On Wed, 2021-11-24 at 19:30 +0800, Xueming Li wrote: > > On Wed, 2021-11-24 at 12:02 +0100, Olivier Matz wrote: > > > From: Xueming Li > > > > > > 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") > > > > > > Signed-off-by: Xueming Li > > > Signed-off-by: Olivier Matz > > > --- > > > bug is specific to 21.11, no need to cc stable@dpdk.org > > > --- > > > app/test/meson.build | 3 + > > > app/test/test_vdev.c | 168 +++++++++++++++++++++++ > > > drivers/bus/auxiliary/auxiliary_params.c | 9 +- > > > drivers/bus/vdev/vdev_params.c | 9 +- > > > 4 files changed, 187 insertions(+), 2 deletions(-) > > > create mode 100644 app/test/test_vdev.c > > > > > > diff --git a/app/test/meson.build b/app/test/meson.build > > > index 961bebc5cb..19009da52e 100644 > > > --- a/app/test/meson.build > > > +++ b/app/test/meson.build > > > @@ -153,6 +153,7 @@ test_sources = files( > > > 'test_trace.c', > > > 'test_trace_register.c', > > > 'test_trace_perf.c', > > > + 'test_vdev.c', > > > 'test_version.c', > > > 'virtual_pmd.c', > > > ) > > > @@ -177,6 +178,7 @@ test_deps = [ > > > 'ipsec', > > > 'lpm', > > > 'member', > > > + 'net_null', > > > > From my experience, CI will fail if build not configured with net_null, > > even we add it here. Let's monitor CI result. > > Sorry, from CI scripts, net_null was there already, no concern. No you are right! I had almost the same comment from David offline. I'm fixing the meson.build to add the test only when RTE_NET_NULL is set. Thanks > > > > > 'node', > > > 'pipeline', > > > 'port', > > > @@ -283,6 +285,7 @@ fast_tests = [ > > > ['service_autotest', true], > > > ['thash_autotest', true], > > > ['trace_autotest', true], > > > + ['vdev_autotest', true], > > > ] > > > > > > # Tests known to have issues or which don't belong in other tests lists. > > > diff --git a/app/test/test_vdev.c b/app/test/test_vdev.c > > > new file mode 100644 > > > index 0000000000..720722c363 > > > --- /dev/null > > > +++ b/app/test/test_vdev.c > > > @@ -0,0 +1,168 @@ > > > +/* SPDX-License-Identifier: BSD-3-Clause > > > + * Copyright 2021 6WIND S.A. > > > + */ > > > + > > > +#include > > > +#include > > > +#include > > > + > > > +#include > > > +#include > > > +#include > > > + > > > +#include "test.h" > > > + > > > +#define TEST_VDEV_KEY_NAME "name" > > > + > > > +static const char * const valid_keys[] = { > > > + TEST_VDEV_KEY_NAME, > > > + NULL, > > > +}; > > > + > > > +static int > > > +cmp_dev_name(const struct rte_device *dev, const void *name) > > > +{ > > > + return strcmp(dev->name, name); > > > +} > > > + > > > +static int > > > +cmp_dev_match(const struct rte_device *dev, const void *_kvlist) > > > +{ > > > + const struct rte_kvargs *kvlist = _kvlist; > > > + const char *key = TEST_VDEV_KEY_NAME; > > > + const char *name; > > > + > > > + /* no kvlist arg, all devices match */ > > > + if (kvlist == NULL) > > > + return 0; > > > + > > > + /* if key is present in kvlist and does not match, filter device */ > > > + name = rte_kvargs_get(kvlist, key); > > > + if (name != NULL && strcmp(name, dev->name)) > > > + return -1; > > > + > > > + return 0; > > > +} > > > + > > > +static struct rte_device * > > > +get_matching_vdev(const char *match_str) > > > +{ > > > + struct rte_bus *vdev_bus = rte_bus_find_by_name("vdev"); > > > + struct rte_kvargs *kvargs = NULL; > > > + struct rte_device *dev; > > > + > > > + if (match_str != NULL) { > > > + kvargs = rte_kvargs_parse(match_str, valid_keys); > > > + if (kvargs == NULL) { > > > + printf("Failed to parse match string\n"); > > > + return NULL; > > > + } > > > + } > > > + > > > + dev = vdev_bus->find_device(NULL, cmp_dev_match, kvargs); > > > + rte_kvargs_free(kvargs); > > > + > > > + return dev; > > > +} > > > + > > > +static int > > > +test_vdev_bus(void) > > > +{ > > > + struct rte_bus *vdev_bus = rte_bus_find_by_name("vdev"); > > > + struct rte_dev_iterator dev_iter = { 0 }; > > > + struct rte_device *dev, *dev0, *dev1; > > > + > > > + /* not supported */ > > > + if (vdev_bus == NULL) > > > + return 0; > > > + > > > + /* create first vdev */ > > > + if (rte_vdev_init("net_null_test0", "") < 0) { > > > + printf("Failed to create vdev net_null_test0\n"); > > > + goto fail; > > > + } > > > + dev0 = vdev_bus->find_device(NULL, cmp_dev_name, "net_null_test0"); > > > + if (dev0 == NULL) { > > > + printf("Cannot find net_null_test0 vdev\n"); > > > + goto fail; > > > + } > > > + > > > + /* create second vdev */ > > > + if (rte_vdev_init("net_null_test1", "") < 0) { > > > + printf("Failed to create vdev net_null_test1\n"); > > > + goto fail; > > > + } > > > + dev1 = vdev_bus->find_device(NULL, cmp_dev_name, "net_null_test1"); > > > + if (dev1 == NULL) { > > > + printf("Cannot find net_null_test1 vdev\n"); > > > + goto fail; > > > + } > > > + > > > + /* try to match vdevs */ > > > + dev = get_matching_vdev("name=net_null_test0"); > > > + if (dev != dev0) { > > > + printf("Cannot match net_null_test0 vdev\n"); > > > + goto fail; > > > + } > > > + > > > + dev = get_matching_vdev("name=net_null_test1"); > > > + if (dev != dev1) { > > > + printf("Cannot match net_null_test1 vdev\n"); > > > + goto fail; > > > + } > > > + > > > + dev = get_matching_vdev("name=unexistant"); > > > + if (dev != NULL) { > > > + printf("Unexistant vdev should not match\n"); > > > + goto fail; > > > + } > > > + > > > + dev = get_matching_vdev(""); > > > + if (dev == NULL || dev == dev1) { > > > + printf("Cannot match any vdev with empty match string\n"); > > > + goto fail; > > > + } > > > + > > > + dev = get_matching_vdev(NULL); > > > + if (dev == NULL || dev == dev1) { > > > + printf("Cannot match any vdev with NULL match string\n"); > > > + goto fail; > > > + } > > > + > > > + /* iterate all vdevs, and ensure we find vdev0 and vdev1 */ > > > + RTE_DEV_FOREACH(dev, "bus=vdev", &dev_iter) { > > > + if (dev == dev0) > > > + dev0 = NULL; > > > + else if (dev == dev1) > > > + dev1 = NULL; > > > + } > > > + if (dev0 != NULL) { > > > + printf("dev0 was not iterated\n"); > > > + goto fail; > > > + } > > > + if (dev1 != NULL) { > > > + printf("dev1 was not iterated\n"); > > > + goto fail; > > > + } > > > + > > > + rte_vdev_uninit("net_null_test0"); > > > + rte_vdev_uninit("net_null_test1"); > > > + > > > + return 0; > > > + > > > +fail: > > > + rte_vdev_uninit("net_null_test0"); > > > + rte_vdev_uninit("net_null_test1"); > > > + return -1; > > > +} > > > + > > > +static int > > > +test_vdev(void) > > > +{ > > > + printf("== test vdev bus ==\n"); > > > + if (test_vdev_bus() < 0) > > > + return -1; > > > + return 0; > > > +} > > > + > > > +REGISTER_TEST_COMMAND(vdev_autotest, test_vdev); > > > diff --git a/drivers/bus/auxiliary/auxiliary_params.c b/drivers/bus/auxiliary/auxiliary_params.c > > > index 8dd8813611..9c08ccdd1b 100644 > > > --- a/drivers/bus/auxiliary/auxiliary_params.c > > > +++ b/drivers/bus/auxiliary/auxiliary_params.c > > > @@ -28,8 +28,15 @@ auxiliary_dev_match(const struct rte_device *dev, > > > { > > > const struct rte_kvargs *kvlist = _kvlist; > > > const char *key = auxiliary_params_keys[RTE_AUXILIARY_PARAM_NAME]; > > > + const char *name; > > > > > > - if (rte_kvargs_get_with_value(kvlist, key, dev->name) == NULL) > > > + /* no kvlist arg, all devices match */ > > > + if (kvlist == NULL) > > > + return 0; > > > + > > > + /* if key is present in kvlist and does not match, filter device */ > > > + name = rte_kvargs_get(kvlist, key); > > > + if (name != NULL && strcmp(name, dev->name)) > > > return -1; > > > > > > return 0; > > > diff --git a/drivers/bus/vdev/vdev_params.c b/drivers/bus/vdev/vdev_params.c > > > index 37d95395e7..3969faf16d 100644 > > > --- a/drivers/bus/vdev/vdev_params.c > > > +++ b/drivers/bus/vdev/vdev_params.c > > > @@ -28,8 +28,15 @@ vdev_dev_match(const struct rte_device *dev, > > > { > > > const struct rte_kvargs *kvlist = _kvlist; > > > const char *key = vdev_params_keys[RTE_VDEV_PARAM_NAME]; > > > + const char *name; > > > > > > - if (rte_kvargs_get_with_value(kvlist, key, dev->name) == NULL) > > > + /* no kvlist arg, all devices match */ > > > + if (kvlist == NULL) > > > + return 0; > > > + > > > + /* if key is present in kvlist and does not match, filter device */ > > > + name = rte_kvargs_get(kvlist, key); > > > + if (name != NULL && strcmp(name, dev->name)) > > > return -1; > > > > > > return 0; > > > > Nice unit test! > > > > Reviewed-by: Xueming Li > > >