Jun Zhu (Intern)
2010-Sep-14 15:31 UTC
RE: [Xen-devel] [PATCH V4] libxl: make libxl communicate with xenstored by socket or xenbus driver
>> + if (!xsh) >> + fprintf(stderr, "cannot connect to xenstore"); > >This should use the logging functions provided, not just write to >stderr.Since libxl__xs_open does not ctx which LIBXL__LOG needs, so I choose to output log with the raw fprintf. libxl: make libxl communicate with xenstored by socket or xenbus driver. This version is remade from the newest staging version, since the libxl interface has been changed. signed-off-by: Jun Zhu <jun.zhu@citrix.com> diff -r cca905a429aa tools/libxl/libxl.c --- a/tools/libxl/libxl.c Tue Sep 14 15:39:36 2010 +0100 +++ b/tools/libxl/libxl.c Tue Sep 14 16:18:00 2010 +0100 @@ -53,12 +53,8 @@ return ERROR_FAIL; } - ctx->xsh = xs_daemon_open(); - if (!ctx->xsh) - ctx->xsh = xs_domain_open(); + ctx->xsh = libxl__xs_open(); if (!ctx->xsh) { - LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, errno, - "cannot connect to xenstore"); xc_interface_close(ctx->xch); return ERROR_FAIL; } @@ -69,7 +65,7 @@ { if (ctx->xch) xc_interface_close(ctx->xch); libxl_version_info_destroy(&ctx->version_info); - if (ctx->xsh) xs_daemon_close(ctx->xsh); + if (ctx->xsh) libxl__xs_close(ctx->xsh); return 0; } @@ -1395,21 +1391,22 @@ { libxl_device_model_starting *starting = for_spawn; char *kvs[3]; - int rc; struct xs_handle *xsh; - xsh = xs_daemon_open(); - /* we mustn''t use the parent''s handle in the child */ - kvs[0] = "image/device-model-pid"; if (asprintf(&kvs[1], "%d", innerchild) < 0) return; kvs[2] = NULL; - rc = xs_writev(xsh, XBT_NULL, starting->dom_path, kvs); - if (rc) - return; - xs_daemon_close(xsh); + /* we mustn''t use the parent''s handle in the child */ + xsh = libxl__xs_open(); + if (!xsh) + goto out; + xs_writev(xsh, XBT_NULL, starting->dom_path, kvs); + libxl__xs_close(xsh); + +out: + free(kvs[1]); } static int libxl_vfb_and_vkb_from_device_model_info(libxl_ctx *ctx, diff -r cca905a429aa tools/libxl/libxl_device.c --- a/tools/libxl/libxl_device.c Tue Sep 14 15:39:36 2010 +0100 +++ b/tools/libxl/libxl_device.c Tue Sep 14 16:18:00 2010 +0100 @@ -405,7 +405,10 @@ unsigned int num; char **l = NULL; - xsh = xs_daemon_open(); + xsh = libxl__xs_open(); + if (!xsh) + return -1; + path = libxl__sprintf(&gc, "/local/domain/0/device-model/%d/state", domid); xs_watch(xsh, path, path); tv.tv_sec = LIBXL_DEVICE_MODEL_START_TIMEOUT; @@ -427,7 +430,7 @@ free(p); xs_unwatch(xsh, path, path); - xs_daemon_close(xsh); + libxl__xs_close(xsh); libxl__free_all(&gc); return rc; again: @@ -444,7 +447,7 @@ } } xs_unwatch(xsh, path, path); - xs_daemon_close(xsh); + libxl__xs_close(xsh); LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Device Model not ready"); libxl__free_all(&gc); return -1; diff -r cca905a429aa tools/libxl/libxl_dom.c --- a/tools/libxl/libxl_dom.c Tue Sep 14 15:39:36 2010 +0100 +++ b/tools/libxl/libxl_dom.c Tue Sep 14 16:18:00 2010 +0100 @@ -326,14 +326,16 @@ snprintf(path, sizeof(path), "/local/domain/0/device-model/%u/logdirty/cmd", domid); - xsh = xs_daemon_open(); + xsh = libxl__xs_open(); + if (!xsh) + return; if (enable) xs_write(xsh, XBT_NULL, path, "enable", strlen("enable")); else xs_write(xsh, XBT_NULL, path, "disable", strlen("disable")); - xs_daemon_close(xsh); + libxl__xs_close(xsh); } static int libxl__domain_suspend_common_callback(void *data) diff -r cca905a429aa tools/libxl/libxl_internal.h --- a/tools/libxl/libxl_internal.h Tue Sep 14 15:39:36 2010 +0100 +++ b/tools/libxl/libxl_internal.h Tue Sep 14 16:18:00 2010 +0100 @@ -138,6 +138,8 @@ _hidden char *libxl__xs_get_dompath(libxl__gc *gc, uint32_t domid); // logs errs _hidden char *libxl__xs_read(libxl__gc *gc, xs_transaction_t t, char *path); _hidden char **libxl__xs_directory(libxl__gc *gc, xs_transaction_t t, char *path, unsigned int *nb); +_hidden struct xs_handle *libxl__xs_open(void); +_hidden void libxl__xs_close(struct xs_handle *xsh); /* from xl_dom */ _hidden int libxl__domain_is_hvm(libxl_ctx *ctx, uint32_t domid); diff -r cca905a429aa tools/libxl/libxl_utils.c --- a/tools/libxl/libxl_utils.c Tue Sep 14 15:39:36 2010 +0100 +++ b/tools/libxl/libxl_utils.c Tue Sep 14 16:18:00 2010 +0100 @@ -367,8 +367,9 @@ int libxl_ctx_postfork(libxl_ctx *ctx) { if (ctx->xsh) xs_daemon_destroy_postfork(ctx->xsh); - ctx->xsh = xs_daemon_open(); - if (!ctx->xsh) return ERROR_FAIL; + ctx->xsh = libxl__xs_open(); + if (!ctx->xsh) + return ERROR_FAIL; return 0; } diff -r cca905a429aa tools/libxl/libxl_xshelp.c --- a/tools/libxl/libxl_xshelp.c Tue Sep 14 15:39:36 2010 +0100 +++ b/tools/libxl/libxl_xshelp.c Tue Sep 14 16:18:00 2010 +0100 @@ -141,3 +141,20 @@ libxl__ptr_add(gc, ret); return ret; } + +struct xs_handle *libxl__xs_open() +{ + struct xs_handle *xsh = NULL; + + xsh = xs_daemon_open(); + if (!xsh) + xsh = xs_domain_open(); + if (!xsh) + fprintf(stderr, "cannot connect to xenstore"); + return xsh; +} + +void libxl__xs_close(struct xs_handle *xsh) +{ + xs_daemon_close(xsh); +} Jun Zhu Citrix Systems UK _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Jackson
2010-Sep-14 16:27 UTC
RE: [Xen-devel] [PATCH V4] libxl: make libxl communicate with xenstored by socket or xenbus driver
Jun Zhu (Intern) writes ("RE: [Xen-devel] [PATCH V4] libxl: make libxl communicate with xenstored by socket or xenbus driver"):> Since libxl__xs_open does not ctx which LIBXL__LOG needs, so I choose to output log with the raw fprintf.Then the right answer is to make it take a ctx parameter, surely ? (And ensure that during initialisation, the logging is initialised before the xenstore connection.) Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2010-Sep-15 08:41 UTC
RE: [Xen-devel] [PATCH V4] libxl: make libxl communicate with xenstored by socket or xenbus driver
On Tue, 2010-09-14 at 17:27 +0100, Ian Jackson wrote:> Jun Zhu (Intern) writes ("RE: [Xen-devel] [PATCH V4] libxl: make libxl communicate with xenstored by socket or xenbus driver"): > > Since libxl__xs_open does not ctx which LIBXL__LOG needs, so I choose to output log with the raw fprintf. > > Then the right answer is to make it take a ctx parameter, surely ?gc->owner contains the surrounding context, doesn''t it? Actually you probably want to use libxl__gc_owner().> And ensure that during initialisation, the logging is initialised > before the xenstore connection.)This seems to be the case in libxl_ctx_init(). Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Jackson
2010-Sep-16 16:35 UTC
RE: [Xen-devel] [PATCH V4] libxl: make libxl communicate with xenstored by socket or xenbus driver
Ian Campbell writes ("RE: [Xen-devel] [PATCH V4] libxl: make libxl communicate with xenstored by socket or xenbus driver"):> On Tue, 2010-09-14 at 17:27 +0100, Ian Jackson wrote: > > Then the right answer is to make it take a ctx parameter, surely ? > > gc->owner contains the surrounding context, doesn''t it?Yes. So passing a gc to libxl__xs_open would be just as good (better probably, for consistency). Jun Zhu''s libxl__xs_open took neither.> > And ensure that during initialisation, the logging is initialised > > before the xenstore connection.) > > This seems to be the case in libxl_ctx_init().I thought it would be; I''m trying to help Jun out by pointing out the things that need to be checked when preparing this patch. Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jun Zhu (Intern)
2010-Sep-16 18:55 UTC
RE: [Xen-devel] [PATCH V4] libxl: make libxl communicate with xenstored by socket or xenbus driver
Hi, Fine. I will pass a gc to libxl__xs_open and make a new patch. Jun Zhu Citrix Systems UK _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jun Zhu (Intern)
2010-Sep-16 19:21 UTC
RE: [Xen-devel] [PATCH V4] libxl: make libxl communicate with xenstored by socket or xenbus driver
Hi, The functions, such as dm_xenstore_record_pid, do not have a ctx pointer in its function parameters. In these functions, if they invoke the libxl__xs_open, it does not have ctx pointer. Should we use the extern ctx directly? I find only the functions in xl_cmdimpl.c use ctx in this way, and the other functions under libxl get the ctx pointer from its function parameter. Jun Zhu Citrix Systems UK ________________________________________ _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2010-Sep-17 08:39 UTC
RE: [Xen-devel] [PATCH V4] libxl: make libxl communicate with xenstored by socket or xenbus driver
On Thu, 2010-09-16 at 20:21 +0100, Jun Zhu (Intern) wrote:> The functions, such as dm_xenstore_record_pid, do not have a ctx > pointer in its function parameters. In these functions, if they invoke > the libxl__xs_open, it does not have ctx pointer. Should we use the > extern ctx directly? I find only the functions in xl_cmdimpl.c use ctx > in this way, and the other functions under libxl get the ctx pointer > from its function parameter.Under no circumstances should libxl try and use a global ctx pointer from the application using libxl. dm_xenstore_record_pid runs in a new process and therefore there is no existing context which can be used. Maybe libxl__xs_open could, as a special case, accept a NULL gc and not log anything in that case? dm_xenstore_record_pid doesn''t log failure today anyway and I presume something in the parent process will eventually log the failure to start the DM. Otherwise I think the choices are: 1) Continue to treat dm_xenstore_record_pid as a special case which uses xs_*_open directly. 2) Allocate a new context in dm_xenstore_record_pid using libxl_ctx_init. It''s not clear what the right logger to use for this would be, maybe some sort of /dev/null logger, in which case we might as well just add a special case to libxl__xs_open which doesn''t log... 3) Rework libxl__spawn_spawn so that the intermediate callback (i.e. dm_xenstore_record_pid) is called in the context of the parent, and hence can have the parent''s context passed to it. I don''t know if this is even possible given the requirements of libxl__spawn_spawn but it might for example involve setting up a pipe to pass the pid from the intermediate process back to the parent, or something along those lines. 3 might be a good thing to do anyway but it''s likely to be tricky to get right and is probably more than you want to get into right now. Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Jackson
2010-Sep-17 15:06 UTC
RE: [Xen-devel] [PATCH V4] libxl: make libxl communicate with xenstored by socket or xenbus driver
Ian Campbell writes ("RE: [Xen-devel] [PATCH V4] libxl: make libxl communicate with xenstored by socket or xenbus driver"):> On Thu, 2010-09-16 at 20:21 +0100, Jun Zhu (Intern) wrote: > > The functions, such as dm_xenstore_record_pid, do not have a ctx > > pointer in its function parameters. In these functions, if they invoke > > the libxl__xs_open, it does not have ctx pointer. Should we use the > > extern ctx directly? I find only the functions in xl_cmdimpl.c use ctx > > in this way, and the other functions under libxl get the ctx pointer > > from its function parameter. > > Under no circumstances should libxl try and use a global ctx pointer > from the application using libxl. > > dm_xenstore_record_pid runs in a new process and therefore there is no > existing context which can be used.I think it is fine to to reuse the ctx from the caller in dm_xenstore_record_pid. Your "new process" test seems to imply problems for any other daemonic processes libxl may need to spawn. If the caller needs to know when libxl forks, we should provide a callback function pointer in the ctx to allow the caller to be told. Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2010-Sep-17 15:13 UTC
RE: [Xen-devel] [PATCH V4] libxl: make libxl communicate with xenstored by socket or xenbus driver
On Fri, 2010-09-17 at 16:06 +0100, Ian Jackson wrote:> Ian Campbell writes ("RE: [Xen-devel] [PATCH V4] libxl: make libxl communicate with xenstored by socket or xenbus driver"): > > On Thu, 2010-09-16 at 20:21 +0100, Jun Zhu (Intern) wrote: > > > The functions, such as dm_xenstore_record_pid, do not have a ctx > > > pointer in its function parameters. In these functions, if they invoke > > > the libxl__xs_open, it does not have ctx pointer. Should we use the > > > extern ctx directly? I find only the functions in xl_cmdimpl.c use ctx > > > in this way, and the other functions under libxl get the ctx pointer > > > from its function parameter. > > > > Under no circumstances should libxl try and use a global ctx pointer > > from the application using libxl. > > > > dm_xenstore_record_pid runs in a new process and therefore there is no > > existing context which can be used. > > I think it is fine to to reuse the ctx from the caller in > dm_xenstore_record_pid. Your "new process" test seems to imply > problems for any other daemonic processes libxl may need to spawn.Hmm, It''s not actually the same context any more since the fork but perhaps that doesn''t really matter. I would have thought that reusing ctx->xsh over a fork would cause all manner of mayhem but it looks like libxl_fork cleverly takes care of that for us. I''d similarly concerned about any other fds linked to the context, are any fds used by the particular xentoollog_logger implementation used by the context going to be alright for example? Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Jackson
2010-Sep-17 15:16 UTC
RE: [Xen-devel] [PATCH V4] libxl: make libxl communicate with xenstored by socket or xenbus driver
Ian Campbell writes ("RE: [Xen-devel] [PATCH V4] libxl: make libxl communicate with xenstored by socket or xenbus driver"):> I''d similarly concerned about any other fds linked to the context, are > any fds used by the particular xentoollog_logger implementation used by > the context going to be alright for example?All the current logger implementations will be fine. If there were a problem it would be no trouble for a logger to do getpid() before each log message.> I would have thought that reusing ctx->xsh over a fork would cause all > manner of mayhem but it looks like libxl_fork cleverly takes care of > that for us.Yes, I did it like that to try to make it harder to accidentally introduce xsh reuse problems. Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel