[PATCH] libxl: include a XLU_Config in _libxlDriverConfig

[PATCH] libxl: include a XLU_Config in _libxlDriverConfig

[Olaf Hering]

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;

Re: [PATCH] libxl: include a XLU_Config in _libxlDriverConfig

[Wei Liu]

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.

Re: [PATCH] libxl: include a XLU_Config in _libxlDriverConfig

[Olaf Hering]

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

Re: [PATCH] libxl: include a XLU_Config in _libxlDriverConfig

[Olaf Hering]

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

Re: [PATCH] libxl: include a XLU_Config in _libxlDriverConfig

[Wei Liu]

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

Re: [PATCH] libxl: include a XLU_Config in _libxlDriverConfig

[Jim Fehlig]

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

Re: [PATCH] libxl: include a XLU_Config in _libxlDriverConfig

[Jim Fehlig]

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

Re: [PATCH] libxl: include a XLU_Config in _libxlDriverConfig

[Olaf Hering]

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

Re: [PATCH] libxl: include a XLU_Config in _libxlDriverConfig

[Wei Liu]

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

Re: [PATCH] libxl: include a XLU_Config in _libxlDriverConfig

[Jim Fehlig]

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

Re: [PATCH] libxl: include a XLU_Config in _libxlDriverConfig

[Jim Fehlig]

On 04/17/2015 11:44 AM, 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 | 2 ++
>   2 files changed, 8 insertions(+)
>
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index 8b76fc7..43712b3 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -1421,6 +1421,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 59389d1..fd2459f 100644
> --- a/src/libxl/libxl_conf.h
> +++ b/src/libxl/libxl_conf.h
> @@ -27,6 +27,7 @@
>   # define LIBXL_CONF_H
>   
>   # include <libxl.h>
> +# include <libxlutil.h>

I needed this too for parsing xl disk config, but until recently it was not 
installed

http://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=8ff079803677b82195addebc0e88f1630cb7354b

I added a check for libxlutil.h to libvirt's configure script. HAVE_LIBXLUTIL_H 
will be defined if it exists.  Currently it is only used in src/xenconfig/xen_xl.c.

Regards,
Jim

>   
>   # include "internal.h"
>   # include "libvirt_internal.h"
> @@ -90,6 +91,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;
>   
>

[PATCH] libxl: include a XLU_Config in _libxlDriverConfig

[Olaf Hering]

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 | 2 ++
 2 files changed, 8 insertions(+)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index 8b76fc7..43712b3 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -1421,6 +1421,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 59389d1..fd2459f 100644
--- a/src/libxl/libxl_conf.h
+++ b/src/libxl/libxl_conf.h
@@ -27,6 +27,7 @@
 # define LIBXL_CONF_H
 
 # include <libxl.h>
+# include <libxlutil.h>
 
 # include "internal.h"
 # include "libvirt_internal.h"
@@ -90,6 +91,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;