Re: [dpdk-dev] [PATCH 1/8] examples/fips_validation: enhance getopt_long usage

[Ibtisam Tariq <ibtisam.tariq@emumba.com>]

Hello David,

IMHO, it cannot be moved to read_uint16 parser.
If we do, we can't verify that the user input value is greater than
UINT16 MAX or not on the overflow data.
> > +                       if (data_room_size == 0 ||
> > +                                       data_room_size > UINT16_MAX) {
> > +                               cryptodev_fips_validate_usage(prgname);
> > +                               return -EINVAL;
> > +                       }

The temp variable:data_room_size is necessary to check the overflow of
the command line argument.

On Thu, Nov 5, 2020 at 1:59 PM David Marchand <david.marchand@redhat.com> wrote:
>
> Hello Ibtisam,
>
> On Wed, Nov 4, 2020 at 11:00 AM Ibtisam Tariq <ibtisam.tariq@emumba.com> wrote:
> > > +               case MBUF_DATAROOM_KEYWORD_NUM:
> > > +               {
> > > +                       uint32_t data_room_size;
> >
> > Here, I don't think we need a temp storage.
> > If the value is invalid, the parsing and then init will fail.
> > You can directly pass &env.mbuf_data_room to parser_read_uint32 and
> > check its value.
> >
> >
> >
> > >
> > > -                               env.mbuf_data_room = data_room_size;
> > > -                       } else {
> > > +                       if (parser_read_uint32(&data_room_size,
> > > +                                       optarg) < 0) {
> > >                                 cryptodev_fips_validate_usage(prgname);
> > >                                 return -EINVAL;
> > >                         }
> > > +
> > > +                       if (data_room_size == 0 ||
> > > +                                       data_room_size > UINT16_MAX) {
> > > +                               cryptodev_fips_validate_usage(prgname);
> > > +                               return -EINVAL;
> > > +                       }
> > > +
> > > +                       env.mbuf_data_room = data_room_size;
> > > +
> > >                         break;
> > > +               }
> >
> > The type of env.mbuf_data_room is uint16_t and the temp variable type
> > is uint32_t. In my opinion, the temp variable size is bigger than
> > env.mbuf_data_room to handle overflow value.
>
> All of it could be moved to a read_uint16 parser.
> WDYT?
>
>
> --
> David Marchand
>


-- 
- Ibtisam