[libvirt] [PATCH] libxl: include a XLU_Config in _libxlDriverConfig
Upcoming changes for vscsi will use libxlutil.so to prepare the configuration for libxl. The helpers needs a xlu struct for logging. Provide one and reuse the existing output as log target. Signed-off-by: Olaf Hering <olaf@aepfle.de> Cc: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_conf.c | 6 ++++++ src/libxl/libxl_conf.h | 7 +++++++ 2 files changed, 13 insertions(+) diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 53f327b..f9bb5ed 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -1454,6 +1454,12 @@ libxlDriverConfigNew(void) goto error; } + cfg->xlu = xlu_cfg_init(cfg->logger_file, "libvirt"); + if (!cfg->xlu) { + VIR_ERROR(_("cannot create xlu for libxenlight, disabling driver")); + goto error; + } + if (libxl_ctx_alloc(&cfg->ctx, LIBXL_VERSION, 0, cfg->logger)) { VIR_ERROR(_("cannot initialize libxenlight context, probably not " "running in a Xen Dom0, disabling driver")); diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h index 5ba1a71..bdc68d4 100644 --- a/src/libxl/libxl_conf.h +++ b/src/libxl/libxl_conf.h @@ -27,6 +27,12 @@ # define LIBXL_CONF_H # include <libxl.h> +#ifdef HAVE_LIBXLUTIL_H +# include <libxlutil.h> +#else +typedef struct XLU_Config XLU_Config; +XLU_Config *xlu_cfg_init(FILE *report, const char *report_filename); +#endif # include "internal.h" # include "libvirt_internal.h" @@ -91,6 +97,7 @@ struct _libxlDriverConfig { /* log stream for driver-wide libxl ctx */ FILE *logger_file; xentoollog_logger *logger; + XLU_Config *xlu; /* libxl ctx for driver wide ops; getVersion, getNodeInfo, ... */ libxl_ctx *ctx;
On Tue, Apr 28, 2015 at 06:40:05AM +0000, Olaf Hering wrote:
Upcoming changes for vscsi will use libxlutil.so to prepare the configuration for libxl. The helpers needs a xlu struct for logging. Provide one and reuse the existing output as log target.
Do you not need to call xlu_cfg_destroy at some point? Wei.
On Tue, May 05, Wei Liu wrote:
On Tue, Apr 28, 2015 at 06:40:05AM +0000, Olaf Hering wrote:
Upcoming changes for vscsi will use libxlutil.so to prepare the configuration for libxl. The helpers needs a xlu struct for logging. Provide one and reuse the existing output as log target.
Do you not need to call xlu_cfg_destroy at some point?
Since libvirtd runs forever there is very little code to undo most things. But I will see if there is any unload function, did not actively look for such thing. Olaf
On Wed, May 06, Olaf Hering wrote:
Since libvirtd runs forever there is very little code to undo most things. But I will see if there is any unload function, did not actively look for such thing.
Looking through libxlStateDriver it seems that libxl and libxlu remains as is. Not sure if thats supposed that way or if perhaps libxl should be fully reinitialized during ->stateReload, and if logfiles should be closed in ->stateCleanup. Olaf
On Wed, May 06, 2015 at 10:58:11AM +0200, Olaf Hering wrote:
On Wed, May 06, Olaf Hering wrote:
Since libvirtd runs forever there is very little code to undo most things. But I will see if there is any unload function, did not actively look for such thing.
Looking through libxlStateDriver it seems that libxl and libxlu remains as is. Not sure if thats supposed that way or if perhaps libxl should be fully reinitialized during ->stateReload, and if logfiles should be closed in ->stateCleanup.
FWIW libxl ctx is destroy in libxlDriverConfigDispose. You can probably just call xlu_cfg_destroy there. Anyway, I'm not a libvirt expert. I just don't like leaking memory. :-) Wei.
Olaf
Wei Liu wrote:
On Wed, May 06, 2015 at 10:58:11AM +0200, Olaf Hering wrote:
On Wed, May 06, Olaf Hering wrote:
Since libvirtd runs forever there is very little code to undo most things. But I will see if there is any unload function, did not actively look for such thing.
Looking through libxlStateDriver it seems that libxl and libxlu remains as is. Not sure if thats supposed that way or if perhaps libxl should be fully reinitialized during ->stateReload, and if logfiles should be closed in ->stateCleanup.
FWIW libxl ctx is destroy in libxlDriverConfigDispose. You can probably just call xlu_cfg_destroy there.
Right. Any additions to the libxlDriverConfig object that needs to be cleaned up should be done in libxlDriverConfigDispose. Regards, Jim
On Wed, May 06, Wei Liu wrote:
On Wed, May 06, 2015 at 10:58:11AM +0200, Olaf Hering wrote:
On Wed, May 06, Olaf Hering wrote:
Since libvirtd runs forever there is very little code to undo most things. But I will see if there is any unload function, did not actively look for such thing.
Looking through libxlStateDriver it seems that libxl and libxlu remains as is. Not sure if thats supposed that way or if perhaps libxl should be fully reinitialized during ->stateReload, and if logfiles should be closed in ->stateCleanup.
FWIW libxl ctx is destroy in libxlDriverConfigDispose. You can probably just call xlu_cfg_destroy there.
In libvirt libxlDriverConfigDispose calls both libxl_ctx_free and xtl_logger_destroy. Both functions check if the input is valid, just xlu_cfg_destroy dereferences the input unconditionally. Should xlu_cfg_destroy be changed to do nothing if it gets passed a NULL pointer? Olaf
On Thu, May 07, 2015 at 10:20:23AM +0200, Olaf Hering wrote:
On Wed, May 06, Wei Liu wrote:
On Wed, May 06, 2015 at 10:58:11AM +0200, Olaf Hering wrote:
On Wed, May 06, Olaf Hering wrote:
Since libvirtd runs forever there is very little code to undo most things. But I will see if there is any unload function, did not actively look for such thing.
Looking through libxlStateDriver it seems that libxl and libxlu remains as is. Not sure if thats supposed that way or if perhaps libxl should be fully reinitialized during ->stateReload, and if logfiles should be closed in ->stateCleanup.
FWIW libxl ctx is destroy in libxlDriverConfigDispose. You can probably just call xlu_cfg_destroy there.
In libvirt libxlDriverConfigDispose calls both libxl_ctx_free and xtl_logger_destroy. Both functions check if the input is valid, just xlu_cfg_destroy dereferences the input unconditionally. Should xlu_cfg_destroy be changed to do nothing if it gets passed a NULL pointer?
That is orthogonal to this patch. I don't particularly care if it's NULL-tolerant or not. In any case, You will need to check validity for older libxl's xlu_cfg_destroy. Wei.
Olaf
Olaf Hering wrote:
On Wed, May 06, Olaf Hering wrote:
Since libvirtd runs forever there is very little code to undo most things. But I will see if there is any unload function, did not actively look for such thing.
Looking through libxlStateDriver it seems that libxl and libxlu remains as is. Not sure if thats supposed that way or if perhaps libxl should be fully reinitialized during ->stateReload, and if logfiles should be closed in ->stateCleanup.
Closing log files, freeing libxl_ctx, etc. is done (indirectly) in stateCleanup. The libxlDriverConfig object is unrefed, which should result in calling libxlDriverConfigDispose(). Jim
Olaf Hering wrote:
Upcoming changes for vscsi will use libxlutil.so to prepare the configuration for libxl. The helpers needs a xlu struct for logging. Provide one and reuse the existing output as log target.
Signed-off-by: Olaf Hering <olaf@aepfle.de> Cc: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_conf.c | 6 ++++++ src/libxl/libxl_conf.h | 7 +++++++ 2 files changed, 13 insertions(+)
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 53f327b..f9bb5ed 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -1454,6 +1454,12 @@ libxlDriverConfigNew(void) goto error; }
+ cfg->xlu = xlu_cfg_init(cfg->logger_file, "libvirt"); + if (!cfg->xlu) { + VIR_ERROR(_("cannot create xlu for libxenlight, disabling driver")); + goto error; + } + if (libxl_ctx_alloc(&cfg->ctx, LIBXL_VERSION, 0, cfg->logger)) { VIR_ERROR(_("cannot initialize libxenlight context, probably not " "running in a Xen Dom0, disabling driver")); diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h index 5ba1a71..bdc68d4 100644 --- a/src/libxl/libxl_conf.h +++ b/src/libxl/libxl_conf.h @@ -27,6 +27,12 @@ # define LIBXL_CONF_H
# include <libxl.h> +#ifdef HAVE_LIBXLUTIL_H +# include <libxlutil.h> +#else +typedef struct XLU_Config XLU_Config; +XLU_Config *xlu_cfg_init(FILE *report, const char *report_filename); +#endif
In addition to the cleanup mentioned by Wei, you'll need to fix the indentation of these preprocessor directives. 'make syntax-check' would have caught that if you had cppi installed. Regards, Jim
participants (3)
-
Jim Fehlig -
Olaf Hering -
Wei Liu