On 4/15/2023 2:38 AM, fengchengwen wrote: > Hi Thomas, Ferruh, > > This patch-set get almost 30% ack by PMD's maintainer. > Could it be applied? and squeeze the patch-set is okay. >
Hi Chengwen, The patch is trivial and safe on its own, so for me having enough ack or not is not what blocks the set. As we discussed before, instead of adding NULL checks to the callbacks, it is better to handle this in the kvargs API level, that discussion is holding this patchset back. According result of discussion we may prefer to not merge this patch at all. > Another thread talk about a change in kvargs API, we could discuss here. > My opinion: > 1) the below PMD has the bug, but there are also many PMD take care of > only key situation. > I think it mainly because the API definition is not detailed, but this > hole was already > fill by commit: 52ab17efdec. Ack > 2) the rte_kvargs_get API, I think we could refine it definition, because > it can't identify > invalid keys and only-keys (both of them return NULL), and I suggest > add one extra parameter: > const char *rte_kvargs_get(const struct rte_kvargs *kvlist, const char > *key, bool *key_exist); > I am not clear why to update `rte_kvargs_get()`, and how to benefit from this update. My suggestion was to update kvargs API to let user of API define if 'key' should have a 'value' or not, similar to done in `getopt()` API. One option can be to update `rte_kvargs_parse()` API to recognize a special char in "const char *const valid_keys[]" to know if value expected or not, and API can do internal checks based on this information. To reduce the impact of change, no special char in the valid_keys string may mean key expects a value (I think this is the most common case), and in this case if "value == NULL" API can return an error. But if there is a special char at the end, lets say '-', key doesn't need a value and having "value == NULL" is accepted. Again this is similar to ':' char in the `getopt()` API, but meaning is reverse. Above is just an option, there can be another solution by to updating `rte_kvargs_process()` API, only this may be more disruptive. Any other solution is welcomed. > Thanks. > > On 2023/3/20 17:20, Chengwen Feng wrote: >> The rte_kvargs_process() was used to parse KV pairs, it also supports >> to parse 'only keys' (e.g. socket_id) type. And the callback function >> parameter 'value' is NULL when parsed 'only keys'. >> >> It may leads to segment fault when parse args with 'only key', this >> patchset fixes rest of them. >> >> Chengwen Feng (44): >> app/pdump: fix segment fault when parse args >> ethdev: fix segment fault when parse args >> net/memif: fix segment fault when parse devargs >> net/pcap: fix segment fault when parse devargs >> net/ring: fix segment fault when parse devargs >> net/sfc: fix segment fault when parse devargs >> net/af_xdp: fix segment fault when parse devargs >> net/ark: fix segment fault when parse devargs >> net/cnxk: fix segment fault when parse devargs >> net/cxgbe: fix segment fault when parse devargs >> net/dpaa2: fix segment fault when parse devargs >> net/ena: fix segment fault when parse devargs >> net/enic: fix segment fault when parse devargs >> net/fm10k: fix segment fault when parse devargs >> net/i40e: fix segment fault when parse devargs >> net/iavf: fix segment fault when parse devargs >> net/ice: fix segment fault when parse devargs >> net/idpf: fix segment fault when parse devargs >> net/ionic: fix segment fault when parse devargs >> net/mana: fix segment fault when parse devargs >> net/mlx4: fix segment fault when parse devargs >> net/mvneta: fix segment fault when parse devargs >> net/mvpp2: fix segment fault when parse devargs >> net/netvsc: fix segment fault when parse devargs >> net/octeontx: fix segment fault when parse devargs >> net/pfe: fix segment fault when parse devargs >> net/qede: fix segment fault when parse devargs >> baseband/la12xx: fix segment fault when parse devargs >> bus/pci: fix segment fault when parse args >> common/mlx5: fix segment fault when parse devargs >> crypto/cnxk: fix segment fault when parse devargs >> crypto/dpaa_sec: fix segment fault when parse devargs >> crypto/dpaa2_sec: fix segment fault when parse devargs >> crypto/mvsam: fix segment fault when parse devargs >> crypto/scheduler: fix segment fault when parse devargs >> dma/dpaa2: fix segment fault when parse devargs >> event/cnxk: fix segment fault when parse devargs >> event/dlb2: fix segment fault when parse devargs >> event/dpaa: fix segment fault when parse devargs >> event/octeontx: fix segment fault when parse devargs >> event/opdl: fix segment fault when parse devargs >> event/sw: fix segment fault when parse devargs >> mempool/cnxk: fix segment fault when parse devargs >> raw/cnxk_gpio: fix segment fault when parse devargs >> >> --- >> v2: according Ferruh's comments: >> fix all 'rte_kvargs_process()' bug instances. >> only judge value validation. >> >> app/pdump/main.c | 12 ++++++ >> drivers/baseband/la12xx/bbdev_la12xx.c | 3 ++ >> drivers/bus/pci/pci_params.c | 2 + >> drivers/common/mlx5/mlx5_common.c | 5 +++ >> drivers/crypto/cnxk/cnxk_cryptodev_devargs.c | 3 ++ >> drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c | 2 + >> drivers/crypto/dpaa_sec/dpaa_sec.c | 3 ++ >> drivers/crypto/mvsam/rte_mrvl_pmd.c | 6 +++ >> drivers/crypto/scheduler/scheduler_pmd.c | 21 +++++++++++ >> drivers/dma/dpaa2/dpaa2_qdma.c | 3 ++ >> drivers/event/cnxk/cnxk_eventdev.c | 6 +++ >> drivers/event/cnxk/cnxk_eventdev.h | 6 +++ >> drivers/event/cnxk/cnxk_tim_evdev.c | 6 +++ >> drivers/event/dlb2/dlb2.c | 5 ++- >> drivers/event/dpaa/dpaa_eventdev.c | 3 ++ >> drivers/event/octeontx/ssovf_evdev.c | 2 + >> drivers/event/opdl/opdl_evdev.c | 9 +++++ >> drivers/event/sw/sw_evdev.c | 12 ++++++ >> drivers/mempool/cnxk/cnxk_mempool.c | 3 ++ >> drivers/net/af_xdp/rte_eth_af_xdp.c | 12 ++++++ >> drivers/net/ark/ark_ethdev.c | 3 ++ >> drivers/net/cnxk/cnxk_ethdev_devargs.c | 39 ++++++++++++++++++++ >> drivers/net/cnxk/cnxk_ethdev_sec.c | 12 ++++++ >> drivers/net/cxgbe/cxgbe_main.c | 3 ++ >> drivers/net/dpaa2/dpaa2_ethdev.c | 3 ++ >> drivers/net/ena/ena_ethdev.c | 6 +++ >> drivers/net/enic/enic_ethdev.c | 6 +++ >> drivers/net/fm10k/fm10k_ethdev.c | 3 ++ >> drivers/net/i40e/i40e_ethdev.c | 15 ++++++++ >> drivers/net/iavf/iavf_ethdev.c | 6 +++ >> drivers/net/ice/ice_dcf_ethdev.c | 6 +++ >> drivers/net/ice/ice_ethdev.c | 6 +++ >> drivers/net/idpf/idpf_ethdev.c | 6 +++ >> drivers/net/ionic/ionic_dev_pci.c | 3 ++ >> drivers/net/mana/mana.c | 3 ++ >> drivers/net/memif/rte_eth_memif.c | 30 +++++++++++++++ >> drivers/net/mlx4/mlx4.c | 3 ++ >> drivers/net/mvneta/mvneta_ethdev.c | 3 ++ >> drivers/net/mvpp2/mrvl_ethdev.c | 3 ++ >> drivers/net/mvpp2/mrvl_qos.c | 6 ++- >> drivers/net/netvsc/hn_ethdev.c | 3 ++ >> drivers/net/octeontx/octeontx_ethdev.c | 3 ++ >> drivers/net/pcap/pcap_ethdev.c | 18 ++++++++- >> drivers/net/pfe/pfe_ethdev.c | 3 ++ >> drivers/net/qede/qede_ethdev.c | 3 ++ >> drivers/net/ring/rte_eth_ring.c | 6 +++ >> drivers/net/sfc/sfc.c | 3 ++ >> drivers/net/sfc/sfc_ev.c | 3 ++ >> drivers/net/sfc/sfc_kvargs.c | 6 +++ >> drivers/raw/cnxk_gpio/cnxk_gpio.c | 6 +++ >> lib/ethdev/rte_class_eth.c | 6 +++ >> 51 files changed, 345 insertions(+), 4 deletions(-) >>
