Re: [Xen-devel] [PATCH V5 29/32] xl: use "libxl-json" format

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Tue, May 20, 2014 at 03:23:26PM +0100, Ian Campbell wrote:
[...]
> > @@ -1787,13 +1770,10 @@ static int handle_domain_death(uint32_t *r_domid,
> >          break;
> >  
> >      case LIBXL_ACTION_ON_SHUTDOWN_RESTART_RENAME:
> > -        reload_domain_config(*r_domid, config_data, config_len);
> >          restart = 2;
> >          break;
> >  
> >      case LIBXL_ACTION_ON_SHUTDOWN_RESTART:
> > -        reload_domain_config(*r_domid, config_data, config_len);
> 
> Why is it not equally necessary to reload the JSON config at this point
> if it exists?
> 

Because domain configuration is loaded in the caller of
handle_domain_death now.

> (commit message should explain this, and in general anywhere that the
> need to reload/refresh the config differs, because my expectation was of
> a more straight forward substitution of reload_domain_config for the new
> function).
> 

No problem.

> > -
> >          restart = 1;
> >          /* fall-through */
> >      case LIBXL_ACTION_ON_SHUTDOWN_DESTROY:
> [...] 
> > -    parse_config_data(config_source, config_data, config_len, &d_config);
> > +    if (config_in_json) {
> > +        char *config_c = config_data;
> > +        if (config_c[config_len-1] != '\0') {
> > +            config_c = xrealloc(config_c, config_len + 1);
> > +            config_c[config_len] = '\0';
> 
> I think this is somewhere that arranging via the protocol that things
> must be null terminated might make sense.
> 

Sure, if I can manage to arrange saving extra "\0" at the end of stream.

> > +    /* If we're doing migration, the domain name was appended with
> > +     * "--incoming" a few lines above. So we need to remove that
> > +     * suffix in the stored configuration.
> 
> It's not possible to save this before we do that appending?
> 

The save is done by libxl, when we create domain. At this point we are
still in xl.

> Or should we not be updating the domain config as we do the renaming at
> the end?
> 

I agree that libxl_domain_rename is a better place to update stored
domain name.

> > +     */
> > +    if (migrate_fd >= 0) {
> > +        libxl_domain_config d;
> > +        int xlen = strlen("--incoming");
> > +        int orig_len;
> > +
> > +        ret = libxl_load_domain_configuration(ctx, domid, &d);
> > +        if (ret) {
> > +            perror("cannot load config data");
> > +            ret = ERROR_FAIL;
> > +            goto error_out;
> > +        }
> > +
> > +        orig_len = strlen(d.c_info.name);
> > +        d.c_info.name = xrealloc(d.c_info.name, orig_len - xlen);
> > +        d.c_info.name[orig_len - xlen] = 0;
> 
> Since the new name is always shorter you could do this simply by
> sticking a \0 in the middle without needing to realloc.

Ack.

But this hunk will be gone if we make libxl_domain_rename capable of
storing new domain name.

> 
> > @@ -3102,22 +3119,18 @@ static void list_domains_details(const 
> > libxl_dominfo *info, int nb_domain)
> >          s = yajl_gen_status_ok;
> >  
> >      for (i = 0; i < nb_domain; i++) {
> > +        libxl_domain_config_init(&d_config);
> >          /* no detailed info available on dom0 */
> >          if (info[i].domid == 0)
> >              continue;
> > -        rc = libxl_userdata_retrieve(ctx, info[i].domid, "xl", &data, 
> > &len);
> > +        rc = libxl_load_domain_configuration(ctx, info[i].domid, 
> > &d_config);
> 
> If a domain was created with xl.old will we not now fail to list details
> of it here?
> 

Don't you need to restart if you install new tool? Is this a valid
usecase?

Wei.

> Ian.

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