[ovs-dev] [PATCH] lib: ofp-parse segfaults if required argument isn't specified

Ben Pfaff blp at nicira.com
Fri Oct 8 23:33:12 UTC 2010


Sure, that's clear.

On Fri, Oct 08, 2010 at 04:20:03PM -0700, Ethan Jackson wrote:
> alright, how about "missing required numeric argument"
> 
> Ethan
> 
> On Fri, Oct 8, 2010 at 4:00 PM, Ben Pfaff <blp at nicira.com> wrote:
> > [Adding dev at openvswitch.org back]
> >
> > The fact that I missed here is that str_to_u32() is implemented in
> > lib/ofp-parse.c.  I thought it was one of the similarly named but
> > different functions in lib/util.c.
> >
> > It's fine now that I see that, although I'd prefer that the message said
> > something about a missing number, since "<NULL>" won't mean anything to
> > most users.
> >
> > Thanks,
> >
> > Ben.
> >
> > On Fri, Oct 08, 2010 at 03:52:21PM -0700, Ethan Jackson wrote:
> >> Well it gets passed in if they don't supply a numeric argument in the
> >> ofproto string when one is required.  For expample "in_port=3
> >> actions=resubmit" instead of "in_port=3 actions=resubmit:4"
> >>
> >>  I think this is the right place to catch it.  If you look at that
> >> function (with the patch applied)
> >>
> >> static uint32_t
> >> str_to_u32(const char *str)
> >> {
> >>     char *tail;
> >>     uint32_t value;
> >>
> >>     if (!str) {
> >>         ovs_fatal(0, "invalid numeric format <NULL>");
> >>     }
> >>
> >>     errno = 0;
> >>     value = strtoul(str, &tail, 0);
> >>     if (errno == EINVAL || errno == ERANGE || *tail) {
> >>         ovs_fatal(0, "invalid numeric format %s", str);
> >>     }
> >>     return value;
> >> }
> >>
> >> The function is already responsible for checking for an "invalid
> >> numeric format".  I think the empty number is a special kind of
> >> "invalid numeric format".
> >>
> >> On Fri, Oct 8, 2010 at 3:40 PM, Ben Pfaff <blp at nicira.com> wrote:
> >> > On Fri, Oct 08, 2010 at 03:17:44PM -0700, Ethan Jackson wrote:
> >> >> Running ovs-ofctl add-flow br0 "in_port=3 actions=resubmit" would
> >> >> segfault instead of reporting an error.
> >> >
> >> > This seems like an odd way to fix the problem.  Why is NULL getting
> >> > passed in?  I'd rather fix that.
> >> >
> >




More information about the dev mailing list