[libvirt] libvirt libxl driver leaking libxl_event's?
Hi Jim, I don't see any calls to libxl_event_dispose in libvirt.git, libxl_event.h says in relation to the event_occurs callback: * event becomes owned by the application and must be freed, either * by event_occurs or later. so does this mean the memory referenced by event is leaked by libxlEventHandler? libxl_event only contains dynamic allocations for LIBXL_EVENT_TYPE_DISK_EJECT which you don't seem to register for so the issue is probably benign but worth noting I think. Ian.
On Thu, 2013-04-11 at 13:46 +0100, Ian Campbell wrote:
Hi Jim,
I don't see any calls to libxl_event_dispose in libvirt.git,
actually I think I meant libxl_event_free, but I can't see that either.
libxl_event.h says in relation to the event_occurs callback: * event becomes owned by the application and must be freed, either * by event_occurs or later. so does this mean the memory referenced by event is leaked by libxlEventHandler?
libxl_event only contains dynamic allocations for LIBXL_EVENT_TYPE_DISK_EJECT which you don't seem to register for so the issue is probably benign but worth noting I think.
Ian.
_______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
On 04/11/2013 07:09 AM, Ian Campbell wrote:
On Thu, 2013-04-11 at 13:46 +0100, Ian Campbell wrote:
Hi Jim,
I don't see any calls to libxl_event_dispose in libvirt.git, actually I think I meant libxl_event_free, but I can't see that either.
Right. I confirmed that was needed with Ian J. a few weeks (months) back, but then never submitted a patch. I've attached one, but noticed that the prototype of the event_occurs callback defines the event parameter as const, causing this error without the lame cast cc1: warnings being treated as errors libxl/libxl_driver.c: In function 'libxlEventHandler': libxl/libxl_driver.c:760: error: passing argument 2 of 'libxl_event_free' discards qualifiers from pointer target type Shouldn't the callback in libxl_event.h just be void (*event_occurs)(void *user, libxl_event *event); But I suppose we can't change that. BTW, the const'ness of event led me to believe it didn't need freed. Regards, Jim
libxl_event.h says in relation to the event_occurs callback: * event becomes owned by the application and must be freed, either * by event_occurs or later. so does this mean the memory referenced by event is leaked by libxlEventHandler?
libxl_event only contains dynamic allocations for LIBXL_EVENT_TYPE_DISK_EJECT which you don't seem to register for so the issue is probably benign but worth noting I think.
Ian.
_______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
On Thu, 2013-04-11 at 20:35 +0100, Jim Fehlig wrote:
On 04/11/2013 07:09 AM, Ian Campbell wrote:
On Thu, 2013-04-11 at 13:46 +0100, Ian Campbell wrote:
Hi Jim,
I don't see any calls to libxl_event_dispose in libvirt.git, actually I think I meant libxl_event_free, but I can't see that either.
Right. I confirmed that was needed with Ian J. a few weeks (months) back, but then never submitted a patch. I've attached one, but noticed that the prototype of the event_occurs callback defines the event parameter as const, causing this error without the lame cast
cc1: warnings being treated as errors libxl/libxl_driver.c: In function 'libxlEventHandler': libxl/libxl_driver.c:760: error: passing argument 2 of 'libxl_event_free' discards qualifiers from pointer target type
Shouldn't the callback in libxl_event.h just be
void (*event_occurs)(void *user, libxl_event *event);
Yes, given the comment that the event becomes owned by and must be freed by the application I think it has to be.
But I suppose we can't change that.
Ugh, you really can't assign a function with a const parameter to a function pointer without the const. Or at least gcc complains I didn't look at the C spec... How annoying! I was hoping that we could just nuke the const in the callback definition and suggest that applications which want to support the 4.2 API keep the cast. We do have the LIBXL_API_VERSION infrastructure to help us in these situations, something like the patch below...
BTW, the const'ness of event led me to believe it didn't need freed.
As well it might! Ian. 8<---------------------- diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h index d18d22c..3a68819 100644 --- a/tools/libxl/libxl.h +++ b/tools/libxl/libxl.h @@ -266,9 +266,9 @@ #include <libxl_uuid.h> #include <_libxl_list.h> -/* API compatibility. Only 0x040200 is supported at this time. */ +/* API compatibility. */ #ifdef LIBXL_API_VERSION -#if LIBXL_API_VERSION != 0x040200 +#if LIBXL_API_VERSION != 0x040200 && LIBXL_API_VERSION != 0x040300 #error Unknown LIBXL_API_VERSION #endif #endif @@ -289,6 +289,16 @@ */ #define LIBXL_HAVE_BUILDINFO_USBDEVICE_LIST 1 +/* + * LIBXL_HAVE_NONCONST_EVENT_OCCURS_EVENT_ARG + * + * This argument was erroneously "const" in the 4.2 release despite + * the requirement for the callback to free the event. + */ +#if LIBXL_API_VERSION != 0x040200 +#define LIBXL_HAVE_NONCONST_EVENT_OCCURS_EVENT_ARG 1 +#endif + /* Functions annotated with LIBXL_EXTERNAL_CALLERS_ONLY may not be * called from within libxl itself. Callers outside libxl, who * do not #include libxl_internal.h, are fine. */ diff --git a/tools/libxl/libxl_event.h b/tools/libxl/libxl_event.h index 51f2721..27a65dc 100644 --- a/tools/libxl/libxl_event.h +++ b/tools/libxl/libxl_event.h @@ -64,7 +64,11 @@ void libxl_event_free(libxl_ctx *ctx, libxl_event *event); typedef struct libxl_event_hooks { uint64_t event_occurs_mask; - void (*event_occurs)(void *user, const libxl_event *event); + void (*event_occurs)(void *user, +#ifndef LIBXL_HAVE_NONCONST_EVENT_OCCURS_EVENT_ARG + const +#endif + libxl_event *event); void (*disaster)(void *user, libxl_event_type type, const char *msg, int errnoval); } libxl_event_hooks;
participants (2)
-
Ian Campbell -
Jim Fehlig