Re: [PATCH v6 07/11] libxl_dm: Pre-open QMP socket for QEMU

[Anthony PERARD <anthony.perard@citrix.com>]

On Fri, Nov 16, 2018 at 11:52:52AM +0000, Ian Jackson wrote:
> > +    r = listen(fd, 1);
> 
> What is the reasoning behind the choice of 1 for the listen queue
> parameter ?

I may have simply copy that from QEMU, or libvirt. They both call listen
with 1. It is probably to allow at least one connect() call on that
socket to succeed before QEMU start. I don't know if 0 is enough.

> >  static int libxl__build_device_model_args_new(libxl__gc *gc,
> >                                          const char *dm, int guest_domid,
> >                                          const libxl_domain_config *guest_config,
> > @@ -944,10 +991,16 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
> >                        GCSPRINTF("%d", guest_domid), NULL);
> >  
> >      flexarray_append(dm_args, "-chardev");
> > -    flexarray_append(dm_args,
> > -                     GCSPRINTF("socket,id=libxl-cmd,"
> > -                               "path=%s,server,nowait",
> > -                               libxl__qemu_qmp_path(gc, guest_domid)));
> > +    if (state->dm_monitor_fd >= 0) {
> > +        flexarray_append(dm_args,
> > +            GCSPRINTF("socket,id=libxl-cmd,fd=%d,server,nowait",
> > +                      state->dm_monitor_fd));
> 
> I think I suggested (IRL perhaps, and perhaps after you posted this)
> that you might add some asserts to the other
> ..._build_device_model_args_... functions.

Will do.

> > @@ -2000,6 +2053,7 @@ void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
> >      if (ret)
> >          goto out;
> >  
> > +    d_state->dm_monitor_fd = -1;
> 
> This function calls libxl__domain_make which you have just changed to
> also set this.  The situation is now very confusing, I think.
> 
> I think it would probably be better to introduce a new function to
> initialise a libxl__domain_build_state, which is called every time one
> comes into existence.  Probably as a separate patch.

I'll attempt to do that.

> > @@ -2408,6 +2470,7 @@ out_close:
> >      if (logfile_w >= 0) close(logfile_w);
> >  out:
> >      if (dm_state_fd >= 0) close(dm_state_fd);
> > +    if (state->dm_monitor_fd >= 0) close(state->dm_monitor_fd);
> 
> I think this cleanup of `state' needs to be split out.  There should
> be a dispose function called everywhere a state ceases to exist.
> See above about _init.

Will do.

-- 
Anthony PERARD

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