>Subject: Re: [EXT] Re: [PATCH v9 3/6] ethdev: add trace points for ethdev (part >two) > >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);
This should work. > >>> 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). >>
