[PATCH 1/4] main: Make -d vid:pid parsing simpler and more flexible

[Stefan Schmidt]

Hello.

On Fri, 2011-05-20 at 17:04, Tormod Volden wrote:
> On Fri, May 20, 2011 at 4:25 PM, Stefan Schmidt wrote:
> >> -static int parse_vendprod(struct usb_vendprod *vp, const char *str)
> >> +static void parse_vendprod(u_int16_t *vendor, u_int16_t *product, const char *str)
> >>  {
> >> -     unsigned long vend, prod;
> >>       const char *colon;
> >>
> >> +     *vendor = strtoul(str, NULL, 16);
> >>       colon = strchr(str, ':');
> >> -     if (!colon || strlen(colon) < 2)
> >> -             return -EINVAL;
> >> -
> >> -     vend = strtoul(str, NULL, 16);
> >> -     prod = strtoul(colon+1, NULL, 16);
> >> -
> >> -     if (vend > 0xffff || prod > 0xffff)
> >> -             return -EINVAL;
> >
> > Any reason you removed the error handling here?
> 
> The values are stuffed directly into *vendor and *product which are
> both u_int16_t so the value can not be higher than 0xffff in any case.

I stand corrected. Bigger then 0xffff for 2 byte would be quite
strange. ;)

> >>               case 'd':
> >> -                     /* Parse device */
> >> -                     if (parse_vendprod(&vendprod, optarg) < 0) {
> >> -                             fprintf(stderr, "unable to parse `%s' as a vendor:product\n", optarg);
> >> -
> >> -                             exit(2);
> >> -                     }
> >> -                     dif->vendor = vendprod.vendor;
> >> -                     dif->product = vendprod.product;
> >> -                     dif->flags |= (DFU_IFF_VENDOR | DFU_IFF_PRODUCT);
> >> +                     /* Parse device ID */
> >> +                     parse_vendprod(&dif->vendor, &dif->product, optarg);
> >
> > Please keep the error handling like the former code.
> 
> For the same reason, parse_vendprod can never fail. If the user
> provides rubbish, the values will be zero, which is equivalent to not
> being filtered on. I instead added the "Filter on" message below so
> that the user can verify his filter expression was recognized.
> 
> One could look at the field length parsed by strtoul() by passing a
> "endptr" instead of NULL as second argument, and from there verify
> that the whole string provided by the user was recognized. However
> this will complicate this small code quite a lot and is IMO not worth
> it. Even if the user gets this wrong, there is not much harm that can
> be done, either no devices will be found because of wrong filter, or
> more than one device because of no filter, which will stop the
> program.
> 
> The only case in which the old code would detect an error was numeric
> overflow (or missing value after the colon, which now is a valid
> case), so I don't consider it much of a loss.

Agreed. Thanks for explaining it. I applied, tested and pushed all
four patches now.

regards
Stefan Schmidt