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

[Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>]

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.
Also, you don't need to set errno if returning the actual error.

(The error handling in this file looks fairly inconsistent anyway but 
let's not make it worse.)
> +
>   /* The hypervisor timeout for the live patching operation is 30 msec,
>    * but it could take some time for the operation to start, so wait twice
>    * that period. */
> @@ -291,8 +357,9 @@ int action_func(int argc, char *argv[], unsigned int idx)
>       char name[XEN_LIVEPATCH_NAME_SIZE];
>       int rc;
>       xen_livepatch_status_t status;
> +    uint64_t flags;
>   
> -    if ( argc != 1 )
> +    if ( argc < 1 )
>       {
>           show_help();
>           return -1;
> @@ -301,7 +368,10 @@ int action_func(int argc, char *argv[], unsigned int idx)
>       if ( idx >= ARRAY_SIZE(action_options) )
>           return -1;
>   
> -    if ( get_name(argc, argv, name) )
> +    if ( get_name(argc--, argv++, name) )
> +        return EINVAL;
> +
> +    if ( get_flags(argc, argv, idx, &flags) )
>           return EINVAL;
>   
>       /* Check initial status. */
> @@ -332,7 +402,7 @@ int action_func(int argc, char *argv[], unsigned int idx)
>       if ( action_options[idx].allow & status.state )
>       {
>           printf("%s %s... ", action_options[idx].verb, name);
> -        rc = action_options[idx].function(xch, name, HYPERVISOR_TIMEOUT_NS);
> +        rc = action_options[idx].function(xch, name, HYPERVISOR_TIMEOUT_NS, 
> flags);
>           if ( rc )
>           {
>               int saved_errno = errno;
> @@ -394,17 +464,23 @@ int action_func(int argc, char *argv[], unsigned int idx)
>   
>   static int load_func(int argc, char *argv[])
>   {
> -    int rc;
> -    char *new_argv[2];
> -    char *path, *name, *lastdot;
> +    int i, rc = ENOMEM;
> +    char *upload_argv[2];
> +    char **apply_argv, *path, *name, *lastdot;
>   
> -    if ( argc != 1 )
> +    if ( argc < 1 )
>       {
>           show_help();
>           return -1;
>       }
> +
> +    /* apply action has <id> [flags] input requirement, which must be 
> constructed */
> +    apply_argv = (char **) malloc(argc * sizeof(*apply_argv));
> +    if ( !apply_argv )
> +        return rc;
> +
>       /* <file> */
> -    new_argv[1] = argv[0];
> +    upload_argv[1] = argv[0];
>   
>       /* Synthesize the <id> */
>       path = strdup(argv[0]);
> @@ -413,16 +489,23 @@ static int load_func(int argc, char *argv[])
>       lastdot = strrchr(name, '.');
>       if ( lastdot != NULL )
>           *lastdot = '\0';
> -    new_argv[0] = name;
> +    upload_argv[0] = name;
> +    apply_argv[0] = name;
>   
> -    rc = upload_func(2 /* <id> <file> */, new_argv);
> +    /* Fill in all user provided flags */
> +    for ( i = 0; i < argc - 1; i++ )
> +        apply_argv[i + 1] = argv[i + 1];
Wouldn't this make the loop body simpler?  i = 1; i < argc;

Or alternatively, just a straight memcpy().

--
Ross Lagerwall

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