Re: [XEN][RFC PATCH v4 16/16] tools/xl: Add new xl command overlay for device tree overlay support

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Tue, Dec 06, 2022 at 10:18:15PM -0800, Vikram Garhwal wrote:
> diff --git a/tools/xl/xl_cmdtable.c b/tools/xl/xl_cmdtable.c
> index 35182ca196..868364c58d 100644
> --- a/tools/xl/xl_cmdtable.c
> +++ b/tools/xl/xl_cmdtable.c
> @@ -630,6 +630,12 @@ const struct cmd_spec cmd_table[] = {
>        "Issue a qemu monitor command to the device model of a domain",
>        "<Domain> <Command>",
>      },
> +    { "dt_overlay",

Command with multiple words are using '-' instead of '_', could you
rename the command to "dt-overlay"?

> +      &main_dt_overlay, 0, 1,
> +      "Add/Remove a device tree overlay",
> +      "add/remove <.dtbo>"
> +      "-h print this help\n"
> +    },
>  };
>  
>  const int cmdtable_len = ARRAY_SIZE(cmd_table);
> diff --git a/tools/xl/xl_vmcontrol.c b/tools/xl/xl_vmcontrol.c
> index 5518c78dc6..b5f04e2741 100644
> --- a/tools/xl/xl_vmcontrol.c
> +++ b/tools/xl/xl_vmcontrol.c
> @@ -1265,6 +1265,54 @@ int main_create(int argc, char **argv)
>      return 0;
>  }
>  
> +int main_dt_overlay(int argc, char **argv)
> +{
> +    const char *overlay_ops = NULL;
> +    const char *overlay_config_file = NULL;
> +    void *overlay_dtb = NULL;
> +    int rc;
> +    uint8_t op;
> +    int overlay_dtb_size = 0;
> +    const int overlay_add_op = 1;
> +    const int overlay_remove_op = 2;
> +
> +    if (argc < 2) {
> +        help("dt_overlay");
> +        return EXIT_FAILURE;
> +    }
> +
> +    overlay_ops = argv[1];
> +    overlay_config_file = argv[2];
> +
> +    if (strcmp(overlay_ops, "add") == 0)
> +        op = overlay_add_op;
> +    else if (strcmp(overlay_ops, "remove") == 0)
> +        op = overlay_remove_op;
> +    else {
> +        fprintf(stderr, "Invalid dt overlay operation\n");
> +        return ERROR_FAIL;

ERROR_FAIL isn't really a value to be used when exiting the programme,
it's value is -3. It's from libxl API.

Instead, could you return EXIT_FAILURE?

> +    }
> +
> +    if (overlay_config_file) {
> +        rc = libxl_read_file_contents(ctx, overlay_config_file,
> +                                      &overlay_dtb, &overlay_dtb_size);
> +
> +        if (rc) {
> +            fprintf(stderr, "failed to read the overlay device tree file 
> %s\n",
> +                    overlay_config_file);
> +            free(overlay_dtb);
> +            return ERROR_FAIL;
> +        }
> +    } else {
> +        fprintf(stderr, "overlay dtbo file not provided\n");
> +        return ERROR_FAIL;
> +    }
> +
> +    rc = libxl_dt_overlay(ctx, overlay_dtb, overlay_dtb_size, op);

Value returned by libxl_*() are going to be negative when there's an
error, so not something to be return by main(). Could you check if
there's an error and return EXIT_FAILURE instead?

> +    free(overlay_dtb);
> +    return rc;
> +}

Thanks,

-- 
Anthony PERARD