Re: [Xen-devel] [PATCH V4 19/24] xl: introduce and use "xl-json" format

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Thu, 2014-05-01 at 13:58 +0100, Wei Liu wrote:
> Originally xl verbatimly copies the domain config file in its user data

I'm not 100% sure but I think the correct phrasing is "... copies the
domain config verbatim into its...". And either s/Originally/Currently/
or s/copies/copied/.

> diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
> index 30bd4bf..f1a1b9b 100644
> --- a/docs/man/xl.pod.1
> +++ b/docs/man/xl.pod.1
> @@ -146,6 +146,11 @@ useful for determining issues with crashing domains and 
> just as a
>  general convenience since you often want to watch the
>  domain boot.
>  
> +=item B<-j>
> +
> +The provided I<configfile> is in JSON format. Cannot be used with "key=value"
> +at the same time.

Do I understand correctly that the intention here is to introduce and
support JSON format configuration files generally, rather than only as
part of the migration protocol?

I don't necessarily object to this but we should we wary of adding new
things to support just because they appear to be easy to do as a side
effect of some other work, there is a maintenance burden associated with
adding this feature, which might potentially be substantial in the long
run.

Do we think there is much call for this feature?

(I'm not vetoing this, I just want to make sure we've thought it
through...)

(having read through the patch, I'm not sure how "free" this is -- the
use of config_in_json seems to have got its tendrils into a lot of
places)

> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index c6a9a0d..970eba2 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -1081,6 +1081,7 @@ void libxl_cpuid_set(libxl_ctx *ctx, uint32_t domid,
>   *
>   *  userid        Data contents
>   *  "xl"          domain config file in xl format, Unix line endings
> + *  "xl-json"     domain config in JSON format generated by xl

"generated by libxl" I think. Could even call it "libxl-json".

>   *  "libvirt-xml" domain config file in libvirt XML format.  See
>   *                http://libvirt.org/formatdomain.html
>   *
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 289ea9a..7443d86 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -110,6 +110,8 @@ static const char migrate_report[]=
>     *            from target to source
>     */
>  
> +#define XL_MANDATORY_FLAG_JSON (1U << 0) /* config data is in JSON format  */
> +#define XL_MANDATORY_FLAG_ALL  (XL_MANDATORY_FLAG_JSON)
>  struct save_file_header {
>      char magic[32]; /* savefileheader_magic */
>      /* All uint32_ts are in domain's byte order. */
> @@ -151,6 +153,7 @@ struct domain_create {
>      int console_autoconnect;
>      int checkpointed_stream;
>      const char *config_file;
> +    int config_in_json;
>      const char *extra_config; /* extra config string */
>      const char *restore_file;
>      int migrate_fd; /* -1 means none */
> @@ -693,6 +696,73 @@ static void parse_top_level_sdl_options(XLU_Config 
> *config,
>      xlu_cfg_replace_string (config, "xauthority", &sdl->xauthority, 0);
>  }
>  
> +static void nic_update_default(libxl_domain_config *d_config)
> +{
> +    int i;
> +    libxl_device_nic *nic;
> +
> +    for (i = 0; i < d_config->num_nics; i++) {
> +        nic = &d_config->nics[i];
> +
> +        if (default_vifscript) {
> +            free(nic->script);
> +            nic->script = strdup(default_vifscript);
> +        }

You can use the replace_string() helper for all these I think.

Except -- what about any explicitly configured script (ie. vif =
['script=blargle']), does this not replace it with the default? Should
this be if !nic->script ?

Do you pickle the original config before libxl on the remote end has set
the defaults? Otherwise how can we distinguish a user supplied script
from a libxl supplied default?

> +static void parse_config_data_json(char *config_data,
> +                                   int config_len,
> +                                   libxl_domain_config *d_config)
> +{
> +    int ret;
> +
> +    if (!config_len) {
> +        fprintf(stderr, "Config data stream empty\n");
> +        exit(1);
> +    }
> +
> +    /* Make sure this string ends with \0 -- the parser expects a NULL
> +     * terminated string.

Perhaps this guarantee could be pushed down into the helper which slurps
the file in for us in the first place?

(that's assuming that making the parser accept a length instead of a
asciz is not possible)

> +     */
> +    if (config_data[config_len-1] != '\0') {
> +        config_data = realloc(config_data, config_len + 1);
> +        if (!config_data) {
> +            fprintf(stderr, "Failed to realloc config_data\n");
> +            exit(1);
> +        }
> +        config_data[config_len] = '\0';
> +    }
> +
> +    ret = libxl_domain_config_from_json(ctx, d_config, config_data);
> +    if (ret) {
> +        fprintf(stderr, "Failed to parse config\n");
> +        exit(1);
> +    }
> +
> +    if (blkdev_start) {
> +        free(d_config->b_info.blkdev_start);
> +        d_config->b_info.blkdev_start = strdup(blkdev_start);
> +    }

replace_string again.

> +
> +    nic_update_default(d_config);
> +}
> +
>  static void parse_config_data(const char *config_source,
>                                const char *config_data,
>                                int config_len,
> @@ -930,6 +1000,8 @@ static void parse_config_data(const char *config_source,
>          b_info->rtc_timeoffset = l;
>  
>      if (dom_info && !xlu_cfg_get_long(config, "vncviewer", &l, 0)) {
> +        fprintf(stderr, "WARNING: \"vncviewer\" option found. It might be 
> removed in future release. "
> +            "Use \"-V\" option of \"xl create\" to automatically spawn 
> vncviewer.\n");

We should either decide this is deprecated or not "might be removed in
the future" is not a helpful thing to say I don't think. It either will
or it won't be removed. Are we feeling brave enough to just remove it?

> @@ -1965,11 +2024,43 @@ static void 
> evdisable_disk_ejects(libxl_evgen_disk_eject **diskws,
>      }
>  }
>  
> +/* Selectively update domain configuration struct. This function is
> + * only used when creating domain.
> + *
> + * src contains a libxl_domain_config that is used by libxl to create
> + * a domain. Presumably libxl fills in relevant information when
> + * creating a domain.

s/Presumably/It assumes that/ (or requires that).


> + *
> + * dst contains a vanilla copy of domain configuration from user

"from the user supplied"

> + * supplied config file. It serves as a template.
> + *
> + * The end result is that dst now contains all relevant information to
> + * reconstruct a domain based on user's configurations and libxl's
> + * decision.

"decisions"

> @@ -2111,12 +2207,22 @@ static uint32_t create_domain(struct domain_create 
> *dom_info)
>              return ERROR_INVAL;
>          }
>          config_source = "<saved>";
> +        config_in_json = !!(hdr.mandatory_flags & XL_MANDATORY_FLAG_JSON);
>      }
>  
>      if (!dom_info->quiet)
>          printf("Parsing config from %s\n", config_source);
>  
> -    parse_config_data(config_source, config_data, config_len, &d_config, 
> dom_info);
> +    if (config_in_json)
> +        parse_config_data_json(config_data, config_len, &d_config);
> +    else
> +        parse_config_data(config_source, config_data, config_len,
> +                          &d_config, dom_info);
> +
> +    /* Save a copy of vanilla domain configuration, as libxl routines
> +     * will fill in more stuffs.

s/stuffs/stuff/.

> +     */
> +    libxl_domain_config_copy(ctx, &d_config_saved, &d_config);
>  
>      if (migrate_fd >= 0) {
>          if (d_config.c_info.name) {
> @@ -3364,6 +3490,8 @@ static void save_domain_core_writeconfig(int fd, const 
> char *source,
>      u32buf.u32 = config_len;
>      ADD_OPTDATA(u32buf.b,    4);
>      ADD_OPTDATA(config_data, config_len);
> +    if (config_in_json)

Should this not always be true when saving? Even if the original was in
xl syntax we've by now converted it?

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel