Re: [Xen-devel] [PATCH v3 02/12] livepatch: Allow to override inter-modules buildid dependency

[Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>]

On 9/17/19 9:27 AM, Wieczorkiewicz, Pawel wrote:
> > On 16. Sep 2019, at 19:01, Ross Lagerwall <ross.lagerwall@xxxxxxxxxx 
> > <mailto:ross.lagerwall@xxxxxxxxxx>> wrote:
> > On 9/16/19 11:59 AM, Pawel Wieczorkiewicz wrote:
> > snip
> > > +/*
> > > + * Parse user provided action flags.
> > > + * This function expects to only receive an array of input 
> > > parameters being flags.
> > > + * Expected action is specified via idx paramater (index of 
> > > flag_options[]).
> > > + */
> > > +static int get_flags(int argc, char *argv[], unsigned int idx, 
> > > uint64_t *flags)
> > > +{
> > > +    int i, j;
> > > +
> > > +    if ( !flags || idx >= ARRAY_SIZE(flag_options) )
> > > +        return -1;
> > > +
> > > +    *flags = 0;
> > > +    for ( i = 0; i < argc; i++ )
> > > +    {
> > > +        for ( j = 0; j < ARRAY_SIZE(flag_options[idx]); j++ )
> > > +        {
> > > +            if ( !flag_options[idx][j].name )
> > > +                goto error;
> > > +
> > > +            if ( !strcmp(flag_options[idx][j].name, argv[i]) )
> > > +            {
> > > +                *flags |= flag_options[idx][j].flag;
> > > +                break;
> > > +            }
> > > +        }
> > > +
> > > +        if ( j == ARRAY_SIZE(flag_options[idx]) )
> > > +            goto error;
> > > +    }
> > > +
> > > +    return 0;
> > > +error:
> > > +    fprintf(stderr, "Unsupported flag: %s.\n", argv[i]);
> > > +    errno = EINVAL;
> > > +    return errno;
> > > +}
> > You return -1 above but +ve errno here. Please make it consistent.
> Well, I understood from the code of the file (e.g. action_func()) that 
> the -1 value indicates a unexpected runtime error (negative val).
> Whereas, positive errno values are expected error to be dealt with.
>
> So:
> <0 - fatal errors
> 0 - ok
>  >0 - errors to be handled
>
> Could you confirm please that I should make get_flags() return only 
> positive errors?
From what I can see, the only positive errors that are "handled" are 
EAGAIN and EBUSY to report EXIT_TIMEOUT and the mixture of returning -1 
and positive errno is a bug. But it's fine to leave it as is for now so it.
> > Also, you don't need to set errno if returning the actual error.
> Honestly, I just copied the code from get_name() and wanted to the 
> get_flags() to follow similar pattern.
Sure.

--
Ross Lagerwall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel