On 2/8/2023 2:15 PM, Ankur Dwivedi wrote: >> Subject: Re: [EXT] Re: [PATCH v9 3/6] ethdev: add trace points for ethdev >> (part >> two) >> >> On 2/8/2023 11:00 AM, Ferruh Yigit wrote: >>> On 2/8/2023 10:42 AM, Ankur Dwivedi wrote: >>>>>> +RTE_TRACE_POINT( >>>>>> + rte_ethdev_trace_set_mc_addr_list, >>>>>> + RTE_TRACE_POINT_ARGS(uint16_t port_id, >>>>>> + const struct rte_ether_addr *mc_addr_set, uint32_t >>>>> nb_mc_addr, >>>>>> + int ret), >>>>>> + rte_trace_point_emit_u16(port_id); >>>>>> + rte_trace_point_emit_ptr(mc_addr_set); >>>>> What about recording this as blob? >>>>> But 'mc_addr_set' is array of addresses, so length needs to be >>>>> 'RTE_ETHER_ADDR_LEN * nb_mc_addr'. >>>> The mc_addr_set pointer can be NULL in rte_eth_dev_set_mc_addr_list. >>>> In that case the blob function will give seg fault. Hence I think blob >>>> cannot >> be used here. >>> Does it make sense to make 'rte_trace_point_emit_blob()' accept NULL >>> and fill all array with 0 in that case to cover this kind of cases? >> >> >> btw, 'rte_trace_point_emit_blob()' already checks for NULL, so expect it >> won't >> give segmentation fault, but won't record the value. > The blob function will be called as > rte_trace_point_emit_blob(mc_addr_set->addr_bytes, len). > If mc_addr_set is NULL then it will result in a segmentation fault. >
Of course trying to access the field 'mc_addr_set->addr_bytes' will cause problem for null pointer, why not: rte_trace_point_emit_blob(mc_addr_set, len); >> Not sure if not recording the value cause problem later when parsing the >> trace >> file. > Wont recording the value is not a issue as the value will not be copied in > trace memory in rte_trace_point_emit_blob() > (lib/eal/include/rte_trace_point.h). >
