> -----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.
> >
> > Thanks,
> > Reshma
