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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Tue, May 06, 2014 at 03:26:42PM +0100, Ian Campbell wrote:
> 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/.
> 

Ah, OK. I will fix this.

> > 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?
> 

Yes. You understanding is correct.

> 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...)
> 

IanJ seemed to fond of this so I added it here. But we can wait, I
think. Next version will be much shorter without this. :-)

> (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".
> 

In my original design this file is actually a JSON representation of xl
config file, but it's not the case anymore. So I think calling it
libxl-json is sufficient.

On the flip side this file is produced by xl (for now) and only used by
xl only, so I think "xl-json" is also approriate.

Not sure which one is better.

> >   *  "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 ?
> 

Haven't thought about this. This is actually just code motion. I think
you just discovered a bug. One more patch added to queue!

> 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?
> 

I will see what I can do.

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

Correct, it's a stream parser. It doesn't accept a length.

> > +     */
> > +    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.
> 

Ack.

> > +
> > +    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?
> 

I think it's quite safe to remove it. This functional regression is in
no way critical and can be easily worked around.

> > @@ -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).
> 
> 

Ack.

> > + *
> > + * 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"
> 

Ack.

> > @@ -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/.
> 

Ack.

> > +     */
> > +    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?
> 

This can be done. It's just that I have not coded it to parse user
supplied config file when saving.

Wei.

> Ian.

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