[dpdk-dev] [PATCH v4 6/9] app/pdump: add pdump tool for packet capturing

Ananyev, Konstantin konstantin.ananyev at intel.com
Thu Jun 2 16:22:27 CEST 2016



> -----Original Message-----
> From: Pattan, Reshma
> Sent: Thursday, June 02, 2016 1:32 PM
> To: Ananyev, Konstantin; dev at dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v4 6/9] app/pdump: add pdump tool for packet capturing
> 
> 
> 
> > -----Original Message-----
> > From: Ananyev, Konstantin
> > Sent: Tuesday, May 31, 2016 6:21 PM
> > To: Pattan, Reshma <reshma.pattan at intel.com>; dev at dpdk.org
> > Subject: RE: [dpdk-dev] [PATCH v4 6/9] app/pdump: add pdump tool for packet
> > capturing
> >
> >
> >
> > > -----Original Message-----
> > > From: Pattan, Reshma
> > > Sent: Tuesday, May 31, 2016 3:50 PM
> > > To: Ananyev, Konstantin; dev at dpdk.org
> > > Subject: RE: [dpdk-dev] [PATCH v4 6/9] app/pdump: add pdump tool for
> > > packet capturing
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Ananyev, Konstantin
> > > > Sent: Friday, May 27, 2016 4:22 PM
> > > > To: Pattan, Reshma <reshma.pattan at intel.com>; dev at dpdk.org
> > > > Cc: Pattan, Reshma <reshma.pattan at intel.com>
> > > > Subject: RE: [dpdk-dev] [PATCH v4 6/9] app/pdump: add pdump tool for
> > > > packet capturing
> > > >
> > > >
> > > > > +static int
> > > > > +parse_num_mbufs(const char *key __rte_unused, const char *value,
> > > > > +void
> > > > > +*extra_args) {
> > > > > +	int n;
> > > > > +	struct pdump_tuples *pt = extra_args;
> > > > > +
> > > > > +	n = atoi(value);
> > > > > +	if (n > 1024)
> > > > > +		pt->total_num_mbufs = (uint16_t) n;
> > > > > +	else {
> > > > > +		printf("total-num-mbufs %d invalid - must be > 1024\n", n);
> > > > > +		return -1;
> > > > > +	}
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > >
> > > >
> > > > You have several parse functions - doing almost the same thing:
> > > > convert string to integer value and then check that this valu is
> > > > within specific range.
> > > > Why not to introduce one function that would accept as extra_args
> > > > pointer to the struct {uint64_t v; uint64_t min; uint64_t max; } So
> > > > inside that function you can check that: v >= min && v < max or so.
> > > > Then you can use that function all over the places.
> > > > Another possibility just have parse function that only does
> > > > conversion without any boundary checking, and make boundary check later
> > in parse_pdump().
> > > > In both cases you can re-use same parse function.
> > > >
> > >
> > > Yes, I do have 4 functions but in all I am not checking min and max
> > > values and log message also differs in each function.  So I would like to retain
> > this as it is now.
> >
> > What for?
> >
> 
> OK, I will make changes to have parse function only for conversion and  boundary checks will be done
> In parse_pdump(). This looks ok to me.
> 
> I agree with rest of the comments and will take care of the same in next revision of patch set.

Ok, thanks.
Konstantin



More information about the dev mailing list