Re: [Xen-devel] [PATCH v5 11/15] libxl_dm: Pre-open QMP socket for QEMU
Anthony PERARD writes ("Re: [PATCH v5 11/15] libxl_dm: Pre-open QMP socket for
QEMU"):
> On Mon, Nov 12, 2018 at 03:14:30PM +, Ian Jackson wrote:
> > Yes, it would have to be initialised along with the other members of
> > libxl__domain_build_state.
>
> I didn't manage to findout where this might be. There is
> libxl__build_pre() but I don't know if it's always called. Is this the
> right place, or is it initialised somewhere else?
Eerrrgh.
Just went off on an absurd wild goose chase to try to answer this
question. I think libxl__domain_build_state predates the current more
rigorous approach to async functions; indeed, I think it predates any
async stuff at all.
I did git-log -G and found this
59f8f46a491c9bdc1ad3e0c5ae4f8b48068d13cd
tools: libxl: remove libxl_domain_build_state from the IDL
AFAICT in that commit it works like this:
* There is a libxl__domain_build_state as a local variable in
each of do_domain_create and libxl__create_stubdom, which are
synchronous functions.
* In each case, the struct is passed by reference into
libxl__domain_build. libxl__domain_build does not initialise it
before passing it to libxl__build_pre. It is filled in as we go.
At some point, libxl_domain_build_state became a member of various
parent state structures. Those parent state structures (and the loose
state) are all initialised to zero at allocation time.
In 50f8ba84a50ebf80dd22067a04062dbaaf2621ff
libxl: arm: Fix build after c/s 74fd984ae
libxl__domain_make started to get a pointer to the
libxl__domain_build_state.
AFAICT I think currently libxl__domain_make is actually the first
thing that touches anything in libxl__domain_build_state. So it
should initialise it.
The layering here is ... a mess.
> Also, now I would probably need to call libxl__pre_open_qmp_socket()
> from libxl__spawn_local_dm() as the d_state (libxl_domain_build_info) is
> const when passed to libxl__build_device_model_args. But that is
> probably fine.
d_state is libxl_domain_build_state isn't it ?
libxl_domain_build_info is d_info or some such.
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v5 11/15] libxl_dm: Pre-open QMP socket for QEMU
Anthony PERARD writes ("Re: [PATCH v5 11/15] libxl_dm: Pre-open QMP socket for
QEMU"):
> On Tue, Oct 16, 2018 at 03:11:03PM +0100, Ian Jackson wrote:
> > Anthony PERARD writes ("[PATCH v5 11/15] libxl_dm: Pre-open QMP socket for
> > QEMU"):
> > > ret = libxl__build_device_model_args(gc, "stubdom-dm", guest_domid,
> > > guest_config, , NULL,
> > > - d_state, NULL);
> > > + d_state, NULL, NULL);
> >
> > Did you consider adding dm_monitor_fd to d_state ?
>
> No, I didn't, I'm not sur what can go into d_state. But that seems
> better that adding an argument to many function prototypes.
I think pretty much anything to do with creating a domain can go in
there.
> On the other hand, I would need to make sure that dm_monitor_fd have a
> proper initial value (-1) when needed.
Yes, it would have to be initialised along with the other members of
libxl__domain_build_state.
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v5 11/15] libxl_dm: Pre-open QMP socket for QEMU
On Tue, Oct 16, 2018 at 03:11:03PM +0100, Ian Jackson wrote:
> Anthony PERARD writes ("[PATCH v5 11/15] libxl_dm: Pre-open QMP socket for
> QEMU"):
> > ret = libxl__build_device_model_args(gc, "stubdom-dm", guest_domid,
> > guest_config, , NULL,
> > - d_state, NULL);
> > + d_state, NULL, NULL);
>
> Did you consider adding dm_monitor_fd to d_state ?
No, I didn't, I'm not sur what can go into d_state. But that seems
better that adding an argument to many function prototypes.
On the other hand, I would need to make sure that dm_monitor_fd have a
proper initial value (-1) when needed.
--
Anthony PERARD
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v5 11/15] libxl_dm: Pre-open QMP socket for QEMU
Anthony PERARD writes ("[PATCH v5 11/15] libxl_dm: Pre-open QMP socket for
QEMU"):
> When starting QEMU with dm_restrict=1, pre-open the QMP socket before
> exec QEMU. That socket will be usefull to findout if QEMU is ready, and
> pre-opening it means that libxl can connect to it without waiting for
> QEMU to create it.
Thanks for this. I have only tiny comments about this. (I checked the
error handling patterns and they seemed conventional and correct.)
FAOD, and I think this is not entirely clear from your commit message:
AIUI the overall effect of this patch is not to enable any new
functionality and not to fix any bug. It just moves the qemu socket
creation from qemu to libxl, but nothing in libxl relies on this yet.
Am I right ? If so please put that in the commit message.
> +static int libxl__pre_open_qmp_socket(libxl__gc *gc, int domid, int *fd_r)
...
> +if (bind(fd, (struct sockaddr*) , sizeof(un)) < 0) {
> +LOGED(ERROR, domid, "bind('%s') failed", path);
> +rc = ERROR_FAIL;
> +goto out;
> +}
From libxl/CODING_STYLE:
* Function calls which might fail (ie most function calls) are
handled by putting the return/status value into a variable, and
then checking it in a separate statement:
char *dompath = libxl__xs_get_dompath(gc, bl->domid);
if (!dompath) { rc = ERROR_FAIL; goto out; }
This needs changing throughout the series, I'm afraid.
> static int libxl__build_device_model_args_new(libxl__gc *gc,
> const char *dm, int guest_domid,
> const libxl_domain_config
> *guest_config,
> char ***args, char ***envs,
> const libxl__domain_build_state
> *state,
> -int *dm_state_fd)
> +int *dm_state_fd, int *dm_monitor_fd)
...
> - GCSPRINTF("socket,id=libxl-cmd,"
> - "path=%s,server,nowait",
> - libxl__qemu_qmp_path(gc, guest_domid)));
> +/* If we have to use dm_restrict, QEMU need to be new enough and will
> have
needs
> + * the new interface where we can pre-open the QMP socket. */
> +if (libxl_defbool_val(b_info->dm_restrict))
> +{
Misplaced brace, should be end of the previous line.
> @@ -1739,10 +1796,11 @@ static int libxl__build_device_model_args(libxl__gc
> *gc,
> case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
> assert(dm_state_fd != NULL);
> assert(*dm_state_fd < 0);
> +assert(dm_monitor_fd != NULL);
Jolly good. But I would be tempted to move or copy this assert to
libxl__pre_open_qmp_socket. What do you think ?
> ret = libxl__build_device_model_args(gc, "stubdom-dm", guest_domid,
> guest_config, , NULL,
> - d_state, NULL);
> + d_state, NULL, NULL);
Did you consider adding dm_monitor_fd to d_state ?
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
