Jun Zhu (Intern)
2010-Sep-02 13:56 UTC
[Xen-devel] [PATCH] libxl: make libxl communicate with xenstored by socket or xenbus driver
Hi, George sent a patch on this problem before, but it was not completed. This patch makes libxl use xenbus to communicate with xenstored if libxl cannot open a socket. There''s a place that does not close fd in the case of failure, which is also fixed in this patch. -----------------------------------------Patch------------------------------------------------------ diff -r eff592364826 tools/libxl/libxl.c --- a/tools/libxl/libxl.c Wed Sep 01 11:23:49 2010 +0100 +++ b/tools/libxl/libxl.c Thu Sep 02 14:51:46 2010 +0100 @@ -1387,10 +1387,8 @@ { 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"; @@ -1398,9 +1396,10 @@ return; kvs[2] = NULL; - rc = xs_writev(xsh, XBT_NULL, starting->dom_path, kvs); - if (rc) - return; + xsh = xs_daemon_open(); + if (!xsh) + xsh = xs_domain_open(); + xs_writev(xsh, XBT_NULL, starting->dom_path, kvs); xs_daemon_close(xsh); } diff -r eff592364826 tools/libxl/libxl_device.c --- a/tools/libxl/libxl_device.c Wed Sep 01 11:23:49 2010 +0100 +++ b/tools/libxl/libxl_device.c Thu Sep 02 14:51:46 2010 +0100 @@ -406,6 +406,8 @@ char **l = NULL; xsh = xs_daemon_open(); + if (!xsh) + xsh = xs_domain_open(); 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; diff -r eff592364826 tools/libxl/libxl_dom.c --- a/tools/libxl/libxl_dom.c Wed Sep 01 11:23:49 2010 +0100 +++ b/tools/libxl/libxl_dom.c Thu Sep 02 14:51:46 2010 +0100 @@ -284,6 +284,8 @@ snprintf(path, sizeof(path), "/local/domain/0/device-model/%u/logdirty/cmd", domid); xsh = xs_daemon_open(); + if (!xsh) + xsh = xs_domain_open(); if (enable) xs_write(xsh, XBT_NULL, path, "enable", strlen("enable")); diff -r eff592364826 tools/libxl/libxl_utils.c --- a/tools/libxl/libxl_utils.c Wed Sep 01 11:23:49 2010 +0100 +++ b/tools/libxl/libxl_utils.c Thu Sep 02 14:51:46 2010 +0100 @@ -368,6 +368,8 @@ int libxl_ctx_postfork(libxl_ctx *ctx) { if (ctx->xsh) xs_daemon_destroy_postfork(ctx->xsh); ctx->xsh = xs_daemon_open(); + if (!ctx->xsh) + ctx->xsh = xs_domain_open(); if (!ctx->xsh) return ERROR_FAIL; return 0; } --------------------------------------END---------------------------------------------- Jun Zhu Citrix Systems UK _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2010-Sep-02 14:59 UTC
Re: [Xen-devel] [PATCH] libxl: make libxl communicate with xenstored by socket or xenbus driver
On Thu, 2010-09-02 at 14:56 +0100, Jun Zhu (Intern) wrote:> Hi, > > George sent a patch on this problem before, but it was not completed. > This patch makes libxl use xenbus to communicate with xenstored if > libxl cannot open a socket.I think rather than repeating the pattern: xsh = xs_daemon_open(); if (!xsh) xsh = xs_domain_open(); everywhere we should add libxl__xenstore_open() instead.> There''s a place that does not close fd in the case of failure, which > is also fixed in this patch.It''s a small patch in this case but generally it is best to put separate fixes in separate patches. Also you need to add a Signed-off-by to your patches. I suspect the maintainers would also appreciate it if you would format the email as DESCRIPTION Signed-off-by: PATCH without additional punctuation or delineation (such as ---Patch----) which they need to edit out when applying. Ian.> > -----------------------------------------Patch------------------------------------------------------ > diff -r eff592364826 tools/libxl/libxl.c > --- a/tools/libxl/libxl.c Wed Sep 01 11:23:49 2010 +0100 > +++ b/tools/libxl/libxl.c Thu Sep 02 14:51:46 2010 +0100 > @@ -1387,10 +1387,8 @@ > { > 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"; > @@ -1398,9 +1396,10 @@ > return; > kvs[2] = NULL; > > - rc = xs_writev(xsh, XBT_NULL, starting->dom_path, kvs); > - if (rc) > - return; > + xsh = xs_daemon_open(); > + if (!xsh) > + xsh = xs_domain_open(); > + xs_writev(xsh, XBT_NULL, starting->dom_path, kvs); > xs_daemon_close(xsh); > } > > diff -r eff592364826 tools/libxl/libxl_device.c > --- a/tools/libxl/libxl_device.c Wed Sep 01 11:23:49 2010 +0100 > +++ b/tools/libxl/libxl_device.c Thu Sep 02 14:51:46 2010 +0100 > @@ -406,6 +406,8 @@ > char **l = NULL; > > xsh = xs_daemon_open(); > + if (!xsh) > + xsh = xs_domain_open(); > 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; > diff -r eff592364826 tools/libxl/libxl_dom.c > --- a/tools/libxl/libxl_dom.c Wed Sep 01 11:23:49 2010 +0100 > +++ b/tools/libxl/libxl_dom.c Thu Sep 02 14:51:46 2010 +0100 > @@ -284,6 +284,8 @@ > snprintf(path, sizeof(path), "/local/domain/0/device-model/%u/logdirty/cmd", domid); > > xsh = xs_daemon_open(); > + if (!xsh) > + xsh = xs_domain_open(); > > if (enable) > xs_write(xsh, XBT_NULL, path, "enable", strlen("enable")); > diff -r eff592364826 tools/libxl/libxl_utils.c > --- a/tools/libxl/libxl_utils.c Wed Sep 01 11:23:49 2010 +0100 > +++ b/tools/libxl/libxl_utils.c Thu Sep 02 14:51:46 2010 +0100 > @@ -368,6 +368,8 @@ > int libxl_ctx_postfork(libxl_ctx *ctx) { > if (ctx->xsh) xs_daemon_destroy_postfork(ctx->xsh); > ctx->xsh = xs_daemon_open(); > + if (!ctx->xsh) > + ctx->xsh = xs_domain_open(); > if (!ctx->xsh) return ERROR_FAIL; > return 0; > } > --------------------------------------END---------------------------------------------- > > > Jun Zhu > Citrix Systems UK > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jun Zhu (Intern)
2010-Sep-06 11:28 UTC
答复: [Xen-devel] [PATCH] libxl: make libxl communicate with xenstored by socket or xenbus driver
# HG changeset patch # User Jun Zhu <Jun.Zhu@citrix.com> # Date 1283771532 -3600 # Node ID f26503f8f56ac0ebbcdbf3aa1dc3a9daeae7cacf # Parent 5f53805b349ecf1ec4fb588e43e8536b5d18b8f5 libxl uses socket or xenbus dev to communicate with xenstored. signed-off-by: Jun Zhu <Jun.Zhu@citrix.com> diff -r 5f53805b349e -r f26503f8f56a tools/libxl/libxl.c --- a/tools/libxl/libxl.c Fri Sep 03 18:44:49 2010 +0100 +++ b/tools/libxl/libxl.c Mon Sep 06 12:12:12 2010 +0100 @@ -53,9 +53,7 @@ return ERROR_FAIL; } - ctx->xsh = xs_daemon_open(); - if (!ctx->xsh) - ctx->xsh = xs_domain_open(); + ctx->xsh = libxl_xs_open(); if (!ctx->xsh) { XL_LOG_ERRNOVAL(ctx, XL_LOG_ERROR, errno, "cannot connect to xenstore"); @@ -69,7 +67,7 @@ { 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; } @@ -1387,10 +1385,8 @@ { 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"; @@ -1398,10 +1394,11 @@ return; kvs[2] = NULL; - rc = xs_writev(xsh, XBT_NULL, starting->dom_path, kvs); - if (rc) - return; - xs_daemon_close(xsh); + xsh = libxl_xs_open(); + if (!xsh) + fprintf(stderr, "cannot connect to xenstore"); + xs_writev(xsh, XBT_NULL, starting->dom_path, kvs); + libxl_xs_close(xsh); } static int libxl_vfb_and_vkb_from_device_model_info(libxl_ctx *ctx, diff -r 5f53805b349e -r f26503f8f56a tools/libxl/libxl_device.c --- a/tools/libxl/libxl_device.c Fri Sep 03 18:44:49 2010 +0100 +++ b/tools/libxl/libxl_device.c Mon Sep 06 12:12:12 2010 +0100 @@ -405,7 +405,10 @@ unsigned int num; char **l = NULL; - xsh = xs_daemon_open(); + xsh = libxl_xs_open(); + if (!xsh) + XL_LOG_ERRNOVAL(ctx, XL_LOG_ERROR, errno, + "cannot connect to xenstore"); 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); XL_LOG(ctx, XL_LOG_ERROR, "Device Model not ready"); libxl_free_all(&gc); return -1; diff -r 5f53805b349e -r f26503f8f56a tools/libxl/libxl_dom.c --- a/tools/libxl/libxl_dom.c Fri Sep 03 18:44:49 2010 +0100 +++ b/tools/libxl/libxl_dom.c Mon Sep 06 12:12:12 2010 +0100 @@ -283,14 +283,16 @@ snprintf(path, sizeof(path), "/local/domain/0/device-model/%u/logdirty/cmd", domid); - xsh = xs_daemon_open(); + xsh = libxl_xs_open(); + if (!xsh) + fprintf(stderr, "cannot connect to xenstore"); 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 core_suspend_callback(void *data) diff -r 5f53805b349e -r f26503f8f56a tools/libxl/libxl_internal.h --- a/tools/libxl/libxl_internal.h Fri Sep 03 18:44:49 2010 +0100 +++ b/tools/libxl/libxl_internal.h Mon Sep 06 12:12:12 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 is_hvm(libxl_ctx *ctx, uint32_t domid); diff -r 5f53805b349e -r f26503f8f56a tools/libxl/libxl_utils.c --- a/tools/libxl/libxl_utils.c Fri Sep 03 18:44:49 2010 +0100 +++ b/tools/libxl/libxl_utils.c Mon Sep 06 12:12:12 2010 +0100 @@ -367,7 +367,10 @@ int libxl_ctx_postfork(libxl_ctx *ctx) { if (ctx->xsh) xs_daemon_destroy_postfork(ctx->xsh); - ctx->xsh = xs_daemon_open(); + ctx->xsh = libxl_xs_open(); + if (!ctx->xsh) + XL_LOG_ERRNOVAL(ctx, XL_LOG_ERROR, errno, + "cannot connect to xenstore"); if (!ctx->xsh) return ERROR_FAIL; return 0; } diff -r 5f53805b349e -r f26503f8f56a tools/libxl/libxl_xshelp.c --- a/tools/libxl/libxl_xshelp.c Fri Sep 03 18:44:49 2010 +0100 +++ b/tools/libxl/libxl_xshelp.c Mon Sep 06 12:12:12 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(); + + return xsh; +} + +void libxl_xs_close(struct xs_handle *xsh) +{ + xs_daemon_close(xsh); +} Jun Zhu Citrix Systems UK ________________________________________ 发件人: Ian Campbell 发送时间: 2010年9月2日 上午 10:59 收件人: Jun Zhu (Intern) 抄送: xen-devel@lists.xensource.com 主题: Re: [Xen-devel] [PATCH] libxl: make libxl communicate with xenstored by socket or xenbus driver On Thu, 2010-09-02 at 14:56 +0100, Jun Zhu (Intern) wrote:> Hi, > > George sent a patch on this problem before, but it was not completed. > This patch makes libxl use xenbus to communicate with xenstored if > libxl cannot open a socket.I think rather than repeating the pattern: xsh = xs_daemon_open(); if (!xsh) xsh = xs_domain_open(); everywhere we should add libxl__xenstore_open() instead.> There's a place that does not close fd in the case of failure, which > is also fixed in this patch.It's a small patch in this case but generally it is best to put separate fixes in separate patches. Also you need to add a Signed-off-by to your patches. I suspect the maintainers would also appreciate it if you would format the email as DESCRIPTION Signed-off-by: PATCH without additional punctuation or delineation (such as ---Patch----) which they need to edit out when applying. Ian.> > -----------------------------------------Patch------------------------------------------------------ > diff -r eff592364826 tools/libxl/libxl.c > --- a/tools/libxl/libxl.c Wed Sep 01 11:23:49 2010 +0100 > +++ b/tools/libxl/libxl.c Thu Sep 02 14:51:46 2010 +0100 > @@ -1387,10 +1387,8 @@ > { > 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"; > @@ -1398,9 +1396,10 @@ > return; > kvs[2] = NULL; > > - rc = xs_writev(xsh, XBT_NULL, starting->dom_path, kvs); > - if (rc) > - return; > + xsh = xs_daemon_open(); > + if (!xsh) > + xsh = xs_domain_open(); > + xs_writev(xsh, XBT_NULL, starting->dom_path, kvs); > xs_daemon_close(xsh); > } > > diff -r eff592364826 tools/libxl/libxl_device.c > --- a/tools/libxl/libxl_device.c Wed Sep 01 11:23:49 2010 +0100 > +++ b/tools/libxl/libxl_device.c Thu Sep 02 14:51:46 2010 +0100 > @@ -406,6 +406,8 @@ > char **l = NULL; > > xsh = xs_daemon_open(); > + if (!xsh) > + xsh = xs_domain_open(); > 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; > diff -r eff592364826 tools/libxl/libxl_dom.c > --- a/tools/libxl/libxl_dom.c Wed Sep 01 11:23:49 2010 +0100 > +++ b/tools/libxl/libxl_dom.c Thu Sep 02 14:51:46 2010 +0100 > @@ -284,6 +284,8 @@ > snprintf(path, sizeof(path), "/local/domain/0/device-model/%u/logdirty/cmd", domid); > > xsh = xs_daemon_open(); > + if (!xsh) > + xsh = xs_domain_open(); > > if (enable) > xs_write(xsh, XBT_NULL, path, "enable", strlen("enable")); > diff -r eff592364826 tools/libxl/libxl_utils.c > --- a/tools/libxl/libxl_utils.c Wed Sep 01 11:23:49 2010 +0100 > +++ b/tools/libxl/libxl_utils.c Thu Sep 02 14:51:46 2010 +0100 > @@ -368,6 +368,8 @@ > int libxl_ctx_postfork(libxl_ctx *ctx) { > if (ctx->xsh) xs_daemon_destroy_postfork(ctx->xsh); > ctx->xsh = xs_daemon_open(); > + if (!ctx->xsh) > + ctx->xsh = xs_domain_open(); > if (!ctx->xsh) return ERROR_FAIL; > return 0; > } > --------------------------------------END---------------------------------------------- > > > Jun Zhu > Citrix Systems UK > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jun Zhu (Intern)
2010-Sep-06 11:35 UTC
RE: [Xen-devel] [PATCH] libxl: make libxl communicate with xenstored by socket or xenbus driver
Hi, The last email''s subject has some problem. sorry. # HG changeset patch # User Jun Zhu <Jun.Zhu@citrix.com> # Date 1283771532 -3600 # Node ID f26503f8f56ac0ebbcdbf3aa1dc3a9daeae7cacf # Parent 5f53805b349ecf1ec4fb588e43e8536b5d18b8f5 libxl uses socket or xenbus dev to communicate with xenstored. signed-off-by: Jun Zhu <Jun.Zhu@citrix.com> diff -r 5f53805b349e -r f26503f8f56a tools/libxl/libxl.c --- a/tools/libxl/libxl.c Fri Sep 03 18:44:49 2010 +0100 +++ b/tools/libxl/libxl.c Mon Sep 06 12:12:12 2010 +0100 @@ -53,9 +53,7 @@ return ERROR_FAIL; } - ctx->xsh = xs_daemon_open(); - if (!ctx->xsh) - ctx->xsh = xs_domain_open(); + ctx->xsh = libxl_xs_open(); if (!ctx->xsh) { XL_LOG_ERRNOVAL(ctx, XL_LOG_ERROR, errno, "cannot connect to xenstore"); @@ -69,7 +67,7 @@ { 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; } @@ -1387,10 +1385,8 @@ { 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"; @@ -1398,10 +1394,11 @@ return; kvs[2] = NULL; - rc = xs_writev(xsh, XBT_NULL, starting->dom_path, kvs); - if (rc) - return; - xs_daemon_close(xsh); + xsh = libxl_xs_open(); + if (!xsh) + fprintf(stderr, "cannot connect to xenstore"); + xs_writev(xsh, XBT_NULL, starting->dom_path, kvs); + libxl_xs_close(xsh); } static int libxl_vfb_and_vkb_from_device_model_info(libxl_ctx *ctx, diff -r 5f53805b349e -r f26503f8f56a tools/libxl/libxl_device.c --- a/tools/libxl/libxl_device.c Fri Sep 03 18:44:49 2010 +0100 +++ b/tools/libxl/libxl_device.c Mon Sep 06 12:12:12 2010 +0100 @@ -405,7 +405,10 @@ unsigned int num; char **l = NULL; - xsh = xs_daemon_open(); + xsh = libxl_xs_open(); + if (!xsh) + XL_LOG_ERRNOVAL(ctx, XL_LOG_ERROR, errno, + "cannot connect to xenstore"); 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); XL_LOG(ctx, XL_LOG_ERROR, "Device Model not ready"); libxl_free_all(&gc); return -1; diff -r 5f53805b349e -r f26503f8f56a tools/libxl/libxl_dom.c --- a/tools/libxl/libxl_dom.c Fri Sep 03 18:44:49 2010 +0100 +++ b/tools/libxl/libxl_dom.c Mon Sep 06 12:12:12 2010 +0100 @@ -283,14 +283,16 @@ snprintf(path, sizeof(path), "/local/domain/0/device-model/%u/logdirty/cmd", domid); - xsh = xs_daemon_open(); + xsh = libxl_xs_open(); + if (!xsh) + fprintf(stderr, "cannot connect to xenstore"); 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 core_suspend_callback(void *data) diff -r 5f53805b349e -r f26503f8f56a tools/libxl/libxl_internal.h --- a/tools/libxl/libxl_internal.h Fri Sep 03 18:44:49 2010 +0100 +++ b/tools/libxl/libxl_internal.h Mon Sep 06 12:12:12 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 is_hvm(libxl_ctx *ctx, uint32_t domid); diff -r 5f53805b349e -r f26503f8f56a tools/libxl/libxl_utils.c --- a/tools/libxl/libxl_utils.c Fri Sep 03 18:44:49 2010 +0100 +++ b/tools/libxl/libxl_utils.c Mon Sep 06 12:12:12 2010 +0100 @@ -367,7 +367,10 @@ int libxl_ctx_postfork(libxl_ctx *ctx) { if (ctx->xsh) xs_daemon_destroy_postfork(ctx->xsh); - ctx->xsh = xs_daemon_open(); + ctx->xsh = libxl_xs_open(); + if (!ctx->xsh) + XL_LOG_ERRNOVAL(ctx, XL_LOG_ERROR, errno, + "cannot connect to xenstore"); if (!ctx->xsh) return ERROR_FAIL; return 0; } diff -r 5f53805b349e -r f26503f8f56a tools/libxl/libxl_xshelp.c --- a/tools/libxl/libxl_xshelp.c Fri Sep 03 18:44:49 2010 +0100 +++ b/tools/libxl/libxl_xshelp.c Mon Sep 06 12:12:12 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(); + + return xsh; +} + +void libxl_xs_close(struct xs_handle *xsh) +{ + xs_daemon_close(xsh); +} Jun Zhu Citrix Systems UK Jun Zhu Citrix Systems UK ________________________________________ From: Ian Campbell Sent: 02 September 2010 10:59 To: Jun Zhu (Intern) Cc: xen-devel@lists.xensource.com Subject: Re: [Xen-devel] [PATCH] libxl: make libxl communicate with xenstored by socket or xenbus driver On Thu, 2010-09-02 at 14:56 +0100, Jun Zhu (Intern) wrote:> Hi, > > George sent a patch on this problem before, but it was not completed. > This patch makes libxl use xenbus to communicate with xenstored if > libxl cannot open a socket.I think rather than repeating the pattern: xsh = xs_daemon_open(); if (!xsh) xsh = xs_domain_open(); everywhere we should add libxl__xenstore_open() instead.> There''s a place that does not close fd in the case of failure, which > is also fixed in this patch.It''s a small patch in this case but generally it is best to put separate fixes in separate patches. Also you need to add a Signed-off-by to your patches. I suspect the maintainers would also appreciate it if you would format the email as DESCRIPTION Signed-off-by: PATCH without additional punctuation or delineation (such as ---Patch----) which they need to edit out when applying. Ian.> > -----------------------------------------Patch------------------------------------------------------ > diff -r eff592364826 tools/libxl/libxl.c > --- a/tools/libxl/libxl.c Wed Sep 01 11:23:49 2010 +0100 > +++ b/tools/libxl/libxl.c Thu Sep 02 14:51:46 2010 +0100 > @@ -1387,10 +1387,8 @@ > { > 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"; > @@ -1398,9 +1396,10 @@ > return; > kvs[2] = NULL; > > - rc = xs_writev(xsh, XBT_NULL, starting->dom_path, kvs); > - if (rc) > - return; > + xsh = xs_daemon_open(); > + if (!xsh) > + xsh = xs_domain_open(); > + xs_writev(xsh, XBT_NULL, starting->dom_path, kvs); > xs_daemon_close(xsh); > } > > diff -r eff592364826 tools/libxl/libxl_device.c > --- a/tools/libxl/libxl_device.c Wed Sep 01 11:23:49 2010 +0100 > +++ b/tools/libxl/libxl_device.c Thu Sep 02 14:51:46 2010 +0100 > @@ -406,6 +406,8 @@ > char **l = NULL; > > xsh = xs_daemon_open(); > + if (!xsh) > + xsh = xs_domain_open(); > 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; > diff -r eff592364826 tools/libxl/libxl_dom.c > --- a/tools/libxl/libxl_dom.c Wed Sep 01 11:23:49 2010 +0100 > +++ b/tools/libxl/libxl_dom.c Thu Sep 02 14:51:46 2010 +0100 > @@ -284,6 +284,8 @@ > snprintf(path, sizeof(path), "/local/domain/0/device-model/%u/logdirty/cmd", domid); > > xsh = xs_daemon_open(); > + if (!xsh) > + xsh = xs_domain_open(); > > if (enable) > xs_write(xsh, XBT_NULL, path, "enable", strlen("enable")); > diff -r eff592364826 tools/libxl/libxl_utils.c > --- a/tools/libxl/libxl_utils.c Wed Sep 01 11:23:49 2010 +0100 > +++ b/tools/libxl/libxl_utils.c Thu Sep 02 14:51:46 2010 +0100 > @@ -368,6 +368,8 @@ > int libxl_ctx_postfork(libxl_ctx *ctx) { > if (ctx->xsh) xs_daemon_destroy_postfork(ctx->xsh); > ctx->xsh = xs_daemon_open(); > + if (!ctx->xsh) > + ctx->xsh = xs_domain_open(); > if (!ctx->xsh) return ERROR_FAIL; > return 0; > } > --------------------------------------END---------------------------------------------- > > > Jun Zhu > Citrix Systems UK > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Jackson
2010-Sep-08 15:31 UTC
Re: ð´: [Xen-devel] [PATCH] libxl: make libxl c ommunicate with xenstored by socket or xenb us driver
Jun Zhu (Intern) writes ("ð´: [Xen-devel] [PATCH] libxl: make libxl c ommunicate with xenstored by socket or xenb us driver"):> libxl uses socket or xenbus dev to communicate with xenstored.Thanks for this patch. I have a few comments:> @@ -1387,10 +1385,8 @@ > { > 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"; > @@ -1398,10 +1394,11 @@ > return; > kvs[2] = NULL; > > - rc = xs_writev(xsh, XBT_NULL, starting->dom_path, kvs); > - if (rc) > - return; > - xs_daemon_close(xsh); > + xsh = libxl_xs_open(); > + if (!xsh) > + fprintf(stderr, "cannot connect to xenstore"); > + xs_writev(xsh, XBT_NULL, starting->dom_path, kvs); > + libxl_xs_close(xsh); > }It''s not clear to me why you''re moving the open call about. Your error handling is incorrect, as passing a null xsh to xs_writev will cause a segfault. Also, while you''re there you should probably fix the memory leak: this function always seems to leak kvs[0].> - xsh = xs_daemon_open(); > + xsh = libxl_xs_open(); > + if (!xsh) > + XL_LOG_ERRNOVAL(ctx, XL_LOG_ERROR, errno, > + "cannot connect to xenstore");Logging is not sufficient; you need to set rc and "goto out", or return an error code, too. This pattern occurs a number of times in your patch. Finally, your patch seems to have been truncated at some point. patch(1) complains: patch: **** malformed patch at line 172: Thanks, Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jun Zhu (Intern)
2010-Sep-08 16:53 UTC
RE: ð´: [Xen-devel] [PATCH] libxl: make libxl c ommunicate with xenstored by socket or xenb us driver
> It''s not clear to me why you''re moving the open call about.This is because the current version does not invoke xs_daemon_close(xsh) if the asprintf is return.> this function always seems to leak kvs[0]kvs[1]? I will try to make a new path according to your suggestion. Thanks! Jun Zhu Citrix Systems UK ________________________________________ From: Ian Jackson Sent: 08 September 2010 11:31 To: Jun Zhu (Intern) Cc: Ian Campbell; xen-devel@lists.xensource.com Subject: Re: ð´: [Xen-devel] [PATCH] libxl: make libxl c ommunicate with xenstored by socket or xenb us driver Jun Zhu (Intern) writes ("ð´: [Xen-devel] [PATCH] libxl: make libxl c ommunicate with xenstored by socket or xenb us driver"):> libxl uses socket or xenbus dev to communicate with xenstored.Thanks for this patch. I have a few comments:> @@ -1387,10 +1385,8 @@ > { > 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"; > @@ -1398,10 +1394,11 @@ > return; > kvs[2] = NULL; > > - rc = xs_writev(xsh, XBT_NULL, starting->dom_path, kvs); > - if (rc) > - return; > - xs_daemon_close(xsh); > + xsh = libxl_xs_open(); > + if (!xsh) > + fprintf(stderr, "cannot connect to xenstore"); > + xs_writev(xsh, XBT_NULL, starting->dom_path, kvs); > + libxl_xs_close(xsh); > }It''s not clear to me why you''re moving the open call about. Your error handling is incorrect, as passing a null xsh to xs_writev will cause a segfault. Also, while you''re there you should probably fix the memory leak: this function always seems to leak kvs[0].> - xsh = xs_daemon_open(); > + xsh = libxl_xs_open(); > + if (!xsh) > + XL_LOG_ERRNOVAL(ctx, XL_LOG_ERROR, errno, > + "cannot connect to xenstore");Logging is not sufficient; you need to set rc and "goto out", or return an error code, too. This pattern occurs a number of times in your patch. Finally, your patch seems to have been truncated at some point. patch(1) complains: patch: **** malformed patch at line 172: Thanks, Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jun Zhu (Intern)
2010-Sep-08 19:20 UTC
RE: [Xen-devel] [PATCH] libxl: make libxl communicate with xenstored by socket or xenbus driver
I make another one. exporting patch: # HG changeset patch # User Jun Zhu <Jun.Zhu@citrix.com> # Date 1283973408 -3600 # Node ID c54e3f3df3ed2edf4e09a004267df81deb999cc5 # Parent 1831912d4109731e78c01be40ec70b5fae804d30 Make libxl use xenbus dev to communicate with xenstored if socket communication does not exists. diff -r 1831912d4109 -r c54e3f3df3ed tools/libxl/libxl.c --- a/tools/libxl/libxl.c Thu Sep 02 19:12:42 2010 +0100 +++ b/tools/libxl/libxl.c Wed Sep 08 20:16:48 2010 +0100 @@ -53,9 +53,7 @@ return ERROR_FAIL; } - ctx->xsh = xs_daemon_open(); - if (!ctx->xsh) - ctx->xsh = xs_domain_open(); + ctx->xsh = libxl_xs_open(); if (!ctx->xsh) { XL_LOG_ERRNOVAL(ctx, XL_LOG_ERROR, errno, "cannot connect to xenstore"); @@ -69,7 +67,7 @@ { 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; } @@ -1387,21 +1385,25 @@ { 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) + { + fprintf(stderr, "cannot connect to xenstore"); + free(kvs[1]); + return; + } + xs_writev(xsh, XBT_NULL, starting->dom_path, kvs); + libxl_xs_close(xsh); + + free(kvs[1]); } static int libxl_vfb_and_vkb_from_device_model_info(libxl_ctx *ctx, diff -r 1831912d4109 -r c54e3f3df3ed tools/libxl/libxl_device.c --- a/tools/libxl/libxl_device.c Thu Sep 02 19:12:42 2010 +0100 +++ b/tools/libxl/libxl_device.c Wed Sep 08 20:16:48 2010 +0100 @@ -405,7 +405,14 @@ unsigned int num; char **l = NULL; - xsh = xs_daemon_open(); + xsh = libxl_xs_open(); + if (!xsh) + { + XL_LOG_ERRNOVAL(ctx, XL_LOG_ERROR, errno, + "cannot connect to xenstore"); + 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 +434,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 +451,7 @@ } } xs_unwatch(xsh, path, path); - xs_daemon_close(xsh); + libxl_xs_close(xsh); XL_LOG(ctx, XL_LOG_ERROR, "Device Model not ready"); libxl_free_all(&gc); return -1; diff -r 1831912d4109 -r c54e3f3df3ed tools/libxl/libxl_dom.c --- a/tools/libxl/libxl_dom.c Thu Sep 02 19:12:42 2010 +0100 +++ b/tools/libxl/libxl_dom.c Wed Sep 08 20:16:48 2010 +0100 @@ -283,14 +283,19 @@ snprintf(path, sizeof(path), "/local/domain/0/device-model/%u/logdirty/cmd", domid); - xsh = xs_daemon_open(); + xsh = libxl_xs_open(); + if (!xsh) + { + fprintf(stderr, "cannot connect to xenstore"); + 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 core_suspend_callback(void *data) diff -r 1831912d4109 -r c54e3f3df3ed tools/libxl/libxl_internal.h --- a/tools/libxl/libxl_internal.h Thu Sep 02 19:12:42 2010 +0100 +++ b/tools/libxl/libxl_internal.h Wed Sep 08 20:16:48 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 is_hvm(libxl_ctx *ctx, uint32_t domid); diff -r 1831912d4109 -r c54e3f3df3ed tools/libxl/libxl_utils.c --- a/tools/libxl/libxl_utils.c Thu Sep 02 19:12:42 2010 +0100 +++ b/tools/libxl/libxl_utils.c Wed Sep 08 20:16:48 2010 +0100 @@ -367,9 +367,14 @@ 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; - return 0; + ctx->xsh = libxl_xs_open(); + if (!ctx->xsh) + { + XL_LOG_ERRNOVAL(ctx, XL_LOG_ERROR, errno, + "cannot connect to xenstore"); + return ERROR_FAIL; + } + return 0; } pid_t libxl_fork(libxl_ctx *ctx) diff -r 1831912d4109 -r c54e3f3df3ed tools/libxl/libxl_xshelp.c --- a/tools/libxl/libxl_xshelp.c Thu Sep 02 19:12:42 2010 +0100 +++ b/tools/libxl/libxl_xshelp.c Wed Sep 08 20:16:48 2010 +0100 @@ -141,3 +141,19 @@ 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(); + + return xsh; +} + +void libxl_xs_close(struct xs_handle *xsh) +{ + xs_daemon_close(xsh); +} Jun Zhu Citrix Systems UK ________________________________________ From: Ian Jackson Sent: 08 September 2010 11:31 To: Jun Zhu (Intern) Cc: Ian Campbell; xen-devel@lists.xensource.com Subject: Re: ð´: [Xen-devel] [PATCH] libxl: make libxl c ommunicate with xenstored by socket or xenb us driver Jun Zhu (Intern) writes ("ð´: [Xen-devel] [PATCH] libxl: make libxl c ommunicate with xenstored by socket or xenb us driver"):> libxl uses socket or xenbus dev to communicate with xenstored.Thanks for this patch. I have a few comments:> @@ -1387,10 +1385,8 @@ > { > 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"; > @@ -1398,10 +1394,11 @@ > return; > kvs[2] = NULL; > > - rc = xs_writev(xsh, XBT_NULL, starting->dom_path, kvs); > - if (rc) > - return; > - xs_daemon_close(xsh); > + xsh = libxl_xs_open(); > + if (!xsh) > + fprintf(stderr, "cannot connect to xenstore"); > + xs_writev(xsh, XBT_NULL, starting->dom_path, kvs); > + libxl_xs_close(xsh); > }It''s not clear to me why you''re moving the open call about. Your error handling is incorrect, as passing a null xsh to xs_writev will cause a segfault. Also, while you''re there you should probably fix the memory leak: this function always seems to leak kvs[0].> - xsh = xs_daemon_open(); > + xsh = libxl_xs_open(); > + if (!xsh) > + XL_LOG_ERRNOVAL(ctx, XL_LOG_ERROR, errno, > + "cannot connect to xenstore");Logging is not sufficient; you need to set rc and "goto out", or return an error code, too. This pattern occurs a number of times in your patch. Finally, your patch seems to have been truncated at some point. patch(1) complains: patch: **** malformed patch at line 172: Thanks, Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Jackson
2010-Sep-09 17:09 UTC
RE: [Xen-devel] [PATCH] libxl: make libxl communicate with xenstored by socket or xenbus driver
Jun Zhu (Intern) writes ("RE: [Xen-devel] [PATCH] libxl: make libxl communicate with xenstored by socket or xenbus driver"):> I make another one.Thanks. (It helps if you say "v2" in the Subject line.)> - 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();This, and some of the other parts of your patch, have what seem to be unintended whitespace and formatting changes. Can you please make sure that: * Your indents are exactly 4 spaces * You don''t put any tabs in (your MUA may mangle them, and anyway we don''t like tabs) * Curly backets (aka braces) should go on the same line as the if and else.> + if (!xsh) > + { > + fprintf(stderr, "cannot connect to xenstore"); > + free(kvs[1]); > + return; > + } > + xs_writev(xsh, XBT_NULL, starting->dom_path, kvs); > + libxl_xs_close(xsh); > + > + free(kvs[1]);This would be better written with the "goto out" idiom, I think ?> - ctx->xsh = xs_daemon_open(); > - if (!ctx->xsh) return ERROR_FAIL; > - return 0; > + ctx->xsh = libxl_xs_open(); > + if (!ctx->xsh) > + { > + XL_LOG_ERRNOVAL(ctx, XL_LOG_ERROR, errno, > + "cannot connect to xenstore");This logging, if it is appropriate, should surely be done in libxl_xs_open, rather than coded out in each call ? Thanks, Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Possibly Parallel Threads
- [PATCH 1/3] libxenlight: Clean up logging arrangements
- [PATCH 0 of 3] Support for VM generation ID save/restore and migrate
- PATCH 0/10: Merge PV framebuffer & console into QEMU
- [PATCH 1 of 4] libxl_device_generic_add: handle NULL fents or bents
- [PATCH 0 of 4] Support for VM generation ID save/restore and migrate