* RE: [PATCH V4] libxl: make libxl communicate with xenstored by socket or xenbus driver
@ 2010-09-14 15:31 Jun Zhu (Intern)
2010-09-14 16:27 ` Ian Jackson
0 siblings, 1 reply; 10+ messages in thread
From: Jun Zhu (Intern) @ 2010-09-14 15:31 UTC (permalink / raw)
To: Ian Jackson; +Cc: Ian Campbell, xen-devel
>> + 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH V4] libxl: make libxl communicate with xenstored by socket or xenbus driver
2010-09-14 15:31 [PATCH V4] libxl: make libxl communicate with xenstored by socket or xenbus driver Jun Zhu (Intern)
@ 2010-09-14 16:27 ` Ian Jackson
2010-09-15 8:41 ` Ian Campbell
0 siblings, 1 reply; 10+ messages in thread
From: Ian Jackson @ 2010-09-14 16:27 UTC (permalink / raw)
To: Jun Zhu (Intern); +Cc: Ian Campbell, xen-devel
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH V4] libxl: make libxl communicate with xenstored by socket or xenbus driver
2010-09-14 16:27 ` Ian Jackson
@ 2010-09-15 8:41 ` Ian Campbell
2010-09-16 16:35 ` Ian Jackson
0 siblings, 1 reply; 10+ messages in thread
From: Ian Campbell @ 2010-09-15 8:41 UTC (permalink / raw)
To: Ian Jackson; +Cc: Jun Zhu (Intern), xen-devel
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH V4] libxl: make libxl communicate with xenstored by socket or xenbus driver
2010-09-15 8:41 ` Ian Campbell
@ 2010-09-16 16:35 ` Ian Jackson
2010-09-16 18:55 ` Jun Zhu (Intern)
2010-09-16 19:21 ` Jun Zhu (Intern)
0 siblings, 2 replies; 10+ messages in thread
From: Ian Jackson @ 2010-09-16 16:35 UTC (permalink / raw)
To: Ian Campbell; +Cc: Jun Zhu (Intern), xen-devel
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH V4] libxl: make libxl communicate with xenstored by socket or xenbus driver
2010-09-16 16:35 ` Ian Jackson
@ 2010-09-16 18:55 ` Jun Zhu (Intern)
2010-09-16 19:21 ` Jun Zhu (Intern)
1 sibling, 0 replies; 10+ messages in thread
From: Jun Zhu (Intern) @ 2010-09-16 18:55 UTC (permalink / raw)
To: Ian Jackson, Ian Campbell; +Cc: xen-devel
Hi,
Fine. I will pass a gc to libxl__xs_open and make a new patch.
Jun Zhu
Citrix Systems UK
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH V4] libxl: make libxl communicate with xenstored by socket or xenbus driver
2010-09-16 16:35 ` Ian Jackson
2010-09-16 18:55 ` Jun Zhu (Intern)
@ 2010-09-16 19:21 ` Jun Zhu (Intern)
2010-09-17 8:39 ` Ian Campbell
1 sibling, 1 reply; 10+ messages in thread
From: Jun Zhu (Intern) @ 2010-09-16 19:21 UTC (permalink / raw)
To: Ian Jackson, Ian Campbell; +Cc: xen-devel
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
________________________________________
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH V4] libxl: make libxl communicate with xenstored by socket or xenbus driver
2010-09-16 19:21 ` Jun Zhu (Intern)
@ 2010-09-17 8:39 ` Ian Campbell
2010-09-17 15:06 ` Ian Jackson
0 siblings, 1 reply; 10+ messages in thread
From: Ian Campbell @ 2010-09-17 8:39 UTC (permalink / raw)
To: Jun Zhu (Intern); +Cc: xen-devel, Ian Jackson
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH V4] libxl: make libxl communicate with xenstored by socket or xenbus driver
2010-09-17 8:39 ` Ian Campbell
@ 2010-09-17 15:06 ` Ian Jackson
2010-09-17 15:13 ` Ian Campbell
0 siblings, 1 reply; 10+ messages in thread
From: Ian Jackson @ 2010-09-17 15:06 UTC (permalink / raw)
To: Ian Campbell; +Cc: Jun Zhu (Intern), xen-devel
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH V4] libxl: make libxl communicate with xenstored by socket or xenbus driver
2010-09-17 15:06 ` Ian Jackson
@ 2010-09-17 15:13 ` Ian Campbell
2010-09-17 15:16 ` Ian Jackson
0 siblings, 1 reply; 10+ messages in thread
From: Ian Campbell @ 2010-09-17 15:13 UTC (permalink / raw)
To: Ian Jackson; +Cc: Jun Zhu (Intern), xen-devel
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH V4] libxl: make libxl communicate with xenstored by socket or xenbus driver
2010-09-17 15:13 ` Ian Campbell
@ 2010-09-17 15:16 ` Ian Jackson
0 siblings, 0 replies; 10+ messages in thread
From: Ian Jackson @ 2010-09-17 15:16 UTC (permalink / raw)
To: Ian Campbell; +Cc: Jun Zhu (Intern), xen-devel
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2010-09-17 15:16 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-09-14 15:31 [PATCH V4] libxl: make libxl communicate with xenstored by socket or xenbus driver Jun Zhu (Intern)
2010-09-14 16:27 ` Ian Jackson
2010-09-15 8:41 ` Ian Campbell
2010-09-16 16:35 ` Ian Jackson
2010-09-16 18:55 ` Jun Zhu (Intern)
2010-09-16 19:21 ` Jun Zhu (Intern)
2010-09-17 8:39 ` Ian Campbell
2010-09-17 15:06 ` Ian Jackson
2010-09-17 15:13 ` Ian Campbell
2010-09-17 15:16 ` Ian Jackson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.