[PATCH 0/4] Add "xl info" command

[PATCH 0/4] Add "xl info" command

[Andre Przywara]

Hi,

as already mentioned in previous emails, the "info" subcommand is 
missing from the xl tool.
The attached patchset adds support for this, extending libxl on the way 
to provide the necessary info only by using own functions.
On my system the output of xm info and xl info was identical, I omitted 
the recent NUMA additions from xl info for now and will provide the
necessary patches later.

Most of the additions are straightforward, just some words on 
libxl_get_version_info():
The xen_version hypercall uses statically allocated memory for passing 
strings (e.g. 1024 Bytes for commandline). To gather all information, 
one has to hypercall multiple times with different arg values, each 
information also has a different (static) result type.
To make this more user friendly in libxl, I created a structure holding 
all information in dynamically allocated pointers. These will be 
malloced and filled on calling libxl_get_version_info() and have to be 
freed by the caller (with libxl_free_version_info()).
To avoid doing several hypercalls and copying unnecessary memory, I used 
a bitmask to select the pieces of information the caller is interested. 
This is especially useful if one wants to know the Xen version or the 
PAGESIZE only. For xl info there is a LIBXL_VERSION_ALL_MASK to get all 
of them.

Please apply!

Regards,
Andre.

-- 
Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
Tel: +49 351 488-3567-12

[PATCH 1/4] libxl: extend physinfo structure

[Andre Przywara]

[-- Attachment #1: Type: text/plain, Size: 345 bytes --]

The libxl version of the physinfo sysctl does not contain some
fields like nr_nodes or capabilities needed for xl info output.
Add them to the structure and the retrieving function.

Signed-off-by: Andre Przywara <andre.przywara@amd.com>

-- 
Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
Tel: +49 351 488-3567-12

[-- Attachment #2: libxl_physinfo_ext.patch --]
[-- Type: text/plain, Size: 1185 bytes --]

diff -r 7ee8bb40200a tools/libxl/libxl.c
--- a/tools/libxl/libxl.c	Thu Apr 15 19:11:16 2010 +0100
+++ b/tools/libxl/libxl.c	Sun Apr 18 14:34:32 2010 +0200
@@ -2344,6 +2344,10 @@
     physinfo->total_pages = xcphysinfo.total_pages;
     physinfo->free_pages = xcphysinfo.free_pages;
     physinfo->scrub_pages = xcphysinfo.scrub_pages;
+    physinfo->nr_nodes = xcphysinfo.nr_nodes;
+    memcpy(physinfo->hw_cap,xcphysinfo.hw_cap, sizeof(physinfo->hw_cap));
+    physinfo->phys_cap = xcphysinfo.capabilities;
+
     return 0;
 }
 
diff -r 7ee8bb40200a tools/libxl/libxl.h
--- a/tools/libxl/libxl.h	Thu Apr 15 19:11:16 2010 +0100
+++ b/tools/libxl/libxl.h	Sun Apr 18 14:34:32 2010 +0200
@@ -415,6 +415,9 @@
     uint64_t *cpumap; /* current cpu's affinities */
 };
 
+#define PHYS_CAP_HVM          1
+#define PHYS_CAP_HVM_DIRECTIO 2
+
 struct libxl_physinfo {
     uint32_t threads_per_core;
     uint32_t cores_per_socket;
@@ -426,6 +429,10 @@
     uint64_t total_pages;
     uint64_t free_pages;
     uint64_t scrub_pages;
+
+    uint32_t nr_nodes;
+    uint32_t hw_cap[8];
+    uint32_t phys_cap;
 };
 
 int libxl_get_physinfo(struct libxl_ctx *ctx, struct libxl_physinfo *physinfo);

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

[PATCH 2/4] libxl: add sched_get_id function

[Andre Przywara]

[-- Attachment #1: Type: text/plain, Size: 306 bytes --]

To get the name of the currently used scheduler, Xen provides a sched_id 
sysctl.
Add a libxl wrapper around the libxc function to query this.

Signed-off-by: Andre Przywara <andre.przywara@amd.com>

-- 
Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
Tel: +49 351 488-3567-12

[-- Attachment #2: libxl_sched_id.patch --]
[-- Type: text/plain, Size: 851 bytes --]

diff -r 7ee8bb40200a tools/libxl/libxl.c
--- a/tools/libxl/libxl.c	Thu Apr 15 19:11:16 2010 +0100
+++ b/tools/libxl/libxl.c	Sun Apr 18 14:37:44 2010 +0200
@@ -2421,3 +2421,13 @@
     }
     return 0;
 }
+
+int libxl_get_sched_id(struct libxl_ctx *ctx)
+{
+    int sched, ret;
+
+    if ((ret = xc_sched_id(ctx->xch, &sched)) != 0)
+        return -ret;
+    return sched;
+}
+
diff -r 7ee8bb40200a tools/libxl/libxl.h
--- a/tools/libxl/libxl.h	Thu Apr 15 19:11:16 2010 +0100
+++ b/tools/libxl/libxl.h	Sun Apr 18 14:37:44 2010 +0200
@@ -441,5 +441,7 @@
 int libxl_set_vcpuaffinity(struct libxl_ctx *ctx, uint32_t domid, uint32_t vcpuid,
                            uint64_t *cpumap, int cpusize);
 int libxl_set_vcpucount(struct libxl_ctx *ctx, uint32_t domid, uint32_t count);
+
+int libxl_get_sched_id(struct libxl_ctx *ctx);
 #endif /* LIBXL_H */
 

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

[PATCH 3/4] libxl: add version_info function

[Andre Przywara]

[-- Attachment #1: Type: text/plain, Size: 428 bytes --]

Xen provides a xen_version hypercall to query the values of several
interesting things (like hypervisor version, commandline used,
actual changeset, etc.). Create a user-friendly and efficient
wrapper around the libxc function to provide values for xl info output.

Signed-off-by: Andre Przywara <andre.przywara@amd.com>

-- 
Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
Tel: +49 351 488-3567-12

[-- Attachment #2: libxl_version_info.patch --]
[-- Type: text/plain, Size: 4609 bytes --]

diff -r 7ee8bb40200a tools/libxl/libxl.c
--- a/tools/libxl/libxl.c	Thu Apr 15 19:11:16 2010 +0100
+++ b/tools/libxl/libxl.c	Sun Apr 18 14:41:48 2010 +0200
@@ -2351,6 +2351,92 @@
     return 0;
 }
 
+int libxl_get_version_info(struct libxl_ctx *ctx,
+                           libxl_version_info *info,
+                           uint32_t query_mask)
+{
+    union {
+        xen_extraversion_t xen_extra;
+        xen_compile_info_t xen_cc;
+        xen_changeset_info_t xen_chgset;
+        xen_capabilities_info_t xen_caps;
+        xen_platform_parameters_t p_parms;
+        xen_commandline_t xen_commandline;
+    } u;
+    long xen_version;
+
+    if (query_mask & LIBXL_VERSION_VERSION_MASK) {
+        xen_version = xc_version(ctx->xch, XENVER_version, NULL);
+        info->xen_version_major = xen_version >> 16;
+        info->xen_version_minor = xen_version & 0xFF;
+        xc_version(ctx->xch, XENVER_extraversion, &u.xen_extra);
+        info->xen_version_extra = strdup(u.xen_extra);
+    } else {
+        info->xen_version_major = 0;
+        info->xen_version_minor = 0;
+        info->xen_version_extra = NULL;
+    }
+
+    if (query_mask & LIBXL_VERSION_COMPILER_MASK) {
+        xc_version(ctx->xch, XENVER_compile_info, &u.xen_cc);
+        info->compiler = strdup(u.xen_cc.compiler);
+        info->compile_by = strdup(u.xen_cc.compile_by);
+        info->compile_domain = strdup(u.xen_cc.compile_domain);
+        info->compile_date = strdup(u.xen_cc.compile_date);
+    } else {
+        info->compiler = NULL;
+        info->compile_by = NULL;
+        info->compile_domain = NULL;
+        info->compile_date = NULL;
+    }
+
+    if (query_mask & LIBXL_VERSION_CAPS_MASK) {
+        xc_version(ctx->xch, XENVER_capabilities, &u.xen_caps);
+        info->capabilities = strdup(u.xen_caps);
+    } else
+        info->capabilities = NULL;
+
+    if (query_mask & LIBXL_VERSION_CHANGESET_MASK) {
+        xc_version(ctx->xch, XENVER_changeset, &u.xen_chgset);
+        info->changeset = strdup(u.xen_chgset);
+    } else 
+        info->changeset = NULL;
+
+    if (query_mask & LIBXL_VERSION_VIRT_START_MASK) {
+        xc_version(ctx->xch, XENVER_platform_parameters, &u.p_parms);
+        info->virt_start = u.p_parms.virt_start;
+    } else
+        info->virt_start = 0;
+
+    if (query_mask & LIBXL_VERSION_PAGESIZE_MASK)
+        info->pagesize = xc_version(ctx->xch, XENVER_pagesize, NULL);
+    else
+        info->pagesize = 0;
+
+    if (query_mask & LIBXL_VERSION_COMMANDLINE_MASK) {
+        xc_version(ctx->xch, XENVER_commandline, &u.xen_commandline);
+        info->commandline = strdup(u.xen_commandline);
+    } else 
+        info->commandline = NULL;
+
+    return 0;
+}
+
+#define FREE_IF_NOT_ZERO(ptr) if((ptr) != NULL) {free(ptr); ptr = NULL;}
+
+void libxl_free_version_info(libxl_version_info *info)
+{
+    FREE_IF_NOT_ZERO(info->xen_version_extra)
+    FREE_IF_NOT_ZERO(info->compiler)
+    FREE_IF_NOT_ZERO(info->compile_by)
+    FREE_IF_NOT_ZERO(info->compile_domain)
+    FREE_IF_NOT_ZERO(info->compile_date)
+    FREE_IF_NOT_ZERO(info->capabilities)
+    FREE_IF_NOT_ZERO(info->changeset)
+    FREE_IF_NOT_ZERO(info->commandline)
+}
+#undef FREE_IF_NOT_ZERO
+
 struct libxl_vcpuinfo *libxl_list_vcpu(struct libxl_ctx *ctx, uint32_t domid,
                                        int *nb_vcpu, int *cpusize)
 {
diff -r 7ee8bb40200a tools/libxl/libxl.h
--- a/tools/libxl/libxl.h	Thu Apr 15 19:11:16 2010 +0100
+++ b/tools/libxl/libxl.h	Sun Apr 18 14:41:48 2010 +0200
@@ -59,6 +59,35 @@
 };
 
 typedef struct {
+    int xen_version_major;
+    int xen_version_minor;
+    char *xen_version_extra;
+    char *compiler;
+    char *compile_by;
+    char *compile_domain;
+    char *compile_date;
+    char *capabilities;
+    char *changeset;
+    unsigned long virt_start;
+    unsigned long pagesize;
+    char *commandline;
+} libxl_version_info;
+
+#define LIBXL_VERSION_VERSION_MASK       (1 <<  0)
+#define LIBXL_VERSION_COMPILER_MASK      (1 <<  1)
+#define LIBXL_VERSION_CAPS_MASK          (1 <<  2)
+#define LIBXL_VERSION_CHANGESET_MASK     (1 <<  3)
+#define LIBXL_VERSION_VIRT_START_MASK    (1 <<  4)
+#define LIBXL_VERSION_PAGESIZE_MASK      (1 <<  5)
+#define LIBXL_VERSION_COMMANDLINE_MASK   (1 <<  6)
+#define LIBXL_VERSION_ALL_MASK          ((1 <<  7) - 1)
+
+int libxl_get_version_info(struct libxl_ctx *ctx,
+                           libxl_version_info *info,
+                           uint32_t query_mask);
+void libxl_free_version_info(libxl_version_info *info);
+
+typedef struct {
     bool hvm;
     bool hap;
     int ssidref;

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

[PATCH 4/4] xl: add "xl info" command

[Andre Przywara]

[-- Attachment #1: Type: text/plain, Size: 376 bytes --]

The info subcommand was missing from the xl tool. Use the new libxl
wrapper functions to create a clone of "xm info". The splitting into
several smaller functions is enspired by the implementation in XendNode.py.

Signed-off-by: Andre Przywara <andre.przywara@amd.com>

-- 
Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
Tel: +49 351 488-3567-12

[-- Attachment #2: xlinfo.patch --]
[-- Type: text/plain, Size: 4405 bytes --]

diff -r 7ee8bb40200a tools/libxl/xl.c
--- a/tools/libxl/xl.c	Thu Apr 15 19:11:16 2010 +0100
+++ b/tools/libxl/xl.c	Sun Apr 18 14:44:46 2010 +0200
@@ -30,6 +30,7 @@
 #include <sys/socket.h>
 #include <sys/select.h>
 #include <arpa/inet.h>
+#include <sys/utsname.h> /* for utsname in xl info */
 #include <xenctrl.h>
 #include <ctype.h>
 #include <inttypes.h>
@@ -2636,6 +2637,122 @@
     exit(0);
 }
 
+static void output_xeninfo(void)
+{
+    libxl_version_info info;
+    int sched_id;
+
+    if (libxl_get_version_info(&ctx, &info, LIBXL_VERSION_ALL_MASK) != 0) {
+        fprintf(stderr, "version_info sysctl failed.\n");
+        return;
+    }
+    if ((sched_id = libxl_get_sched_id(&ctx)) < 0) {
+        fprintf(stderr, "get_sched_id sysctl failed.\n");
+        return;
+    }
+
+    printf("xen_major              : %d\n", info.xen_version_major);
+    printf("xen_minor              : %d\n", info.xen_version_minor);
+    printf("xen_extra              : %s\n", info.xen_version_extra);
+    printf("xen_caps               : %s\n", info.capabilities);
+    printf("xen_scheduler          : %s\n",
+        sched_id == XEN_SCHEDULER_SEDF ? "sedf" :
+        sched_id == XEN_SCHEDULER_CREDIT ? "credit" :
+        sched_id == XEN_SCHEDULER_CREDIT2 ? "credit2" : "unknown");
+    printf("xen_pagesize           : %lu\n", info.pagesize);
+    printf("platform_params        : virt_start=0x%lx\n", info.virt_start);
+    printf("xen_changeset          : %s\n", info.changeset);
+    printf("xen_commandline        : %s\n", info.commandline);
+    printf("cc_compiler            : %s\n", info.compiler);
+    printf("cc_compile_by          : %s\n", info.compile_by);
+    printf("cc_compile_domain      : %s\n", info.compile_domain);
+    printf("cc_compile_date        : %s\n", info.compile_date);
+
+    libxl_free_version_info(&info);
+    return;
+}
+
+static void output_nodeinfo(void)
+{
+    struct utsname utsbuf;
+
+    uname(&utsbuf);
+
+    printf("host                   : %s\n", utsbuf.nodename);
+    printf("release                : %s\n", utsbuf.release);
+    printf("version                : %s\n", utsbuf.version);
+    printf("machine                : %s\n", utsbuf.machine);
+
+    return;
+}
+
+static void output_physinfo(void)
+{
+    struct libxl_physinfo info;
+    libxl_version_info vinfo;
+    unsigned int i;
+
+    if (libxl_get_physinfo(&ctx, &info) != 0) {
+        fprintf(stderr, "libxl_physinfo failed.\n");
+        return;
+    }
+
+    printf("nr_cpus                : %d\n", info.nr_cpus);
+    printf("nr_nodes               : %d\n", info.nr_nodes);
+    printf("cores_per_socket       : %d\n", info.cores_per_socket);
+    printf("threads_per_core       : %d\n", info.threads_per_core);
+    printf("cpu_mhz                : %d\n", info.cpu_khz / 1000);
+    printf("hw_caps                : ");
+    for (i = 0; i < 8; i++)
+        printf("%08x%c", info.hw_cap[i], i < 7 ? ':' : '\n');
+    printf("virt_caps              :");
+    if (info.phys_cap & PHYS_CAP_HVM)
+        printf(" hvm");
+    if (info.phys_cap & PHYS_CAP_HVM_DIRECTIO)
+        printf(" hvm_directio");
+    printf("\n");
+    libxl_get_version_info(&ctx, &vinfo, LIBXL_VERSION_PAGESIZE_MASK);
+    i = (1 << 20) / vinfo.pagesize;
+    libxl_free_version_info(&vinfo);
+    printf("total_memory           : %lu\n", info.total_pages / i);
+    printf("free_memory            : %lu\n", info.free_pages / i);
+
+    return;
+}
+
+void info(int verbose)
+{
+    output_nodeinfo();
+
+    output_physinfo();
+
+    output_xeninfo();
+
+    printf("xend_config_format     : 4\n");
+
+    return;
+}
+
+void main_info(int argc, char **argv)
+{
+    int opt, verbose;
+
+    verbose = 0;
+    while ((opt = getopt(argc, argv, "h")) != -1) {
+        switch (opt) {
+        case 'h':
+            help("vcpu-list");
+            exit(0);
+        default:
+            fprintf(stderr, "option `%c' not supported.\n", opt);
+            break;
+        }
+    }
+
+    info(verbose);
+    exit(0);
+}
+
 int main(int argc, char **argv)
 {
     if (argc < 2) {
@@ -2696,6 +2813,8 @@
         main_vcpupin(argc - 1, argv + 1);
     } else if (!strcmp(argv[1], "vcpu-set")) {
         main_vcpuset(argc - 1, argv + 1);
+    } else if (!strcmp(argv[1], "info")) {
+        main_info(argc - 1, argv + 1);
     } else if (!strcmp(argv[1], "help")) {
         if (argc > 2)
             help(argv[2]);

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH 1/4] libxl: extend physinfo structure

[Vincent Hanquez]

On 18/04/10 22:23, Andre Przywara wrote:
> The libxl version of the physinfo sysctl does not contain some
> fields like nr_nodes or capabilities needed for xl info output.
> Add them to the structure and the retrieving function.
>
> Signed-off-by: Andre Przywara<andre.przywara@amd.com>

I think, It would be much nicer to provide the phys_caps already decoded 
instead of copying the xc value as is.

so what about something like having phys_cap_hvm and 
phys_cap_hvm_directio field directly in the structure ? If things 
extends, we should add new field in libxl anyway.

-- 
Vincent

Re: [PATCH 2/4] libxl: add sched_get_id function

[Vincent Hanquez]

On 18/04/10 22:25, Andre Przywara wrote:
> To get the name of the currently used scheduler, Xen provides a sched_id
> sysctl.
> Add a libxl wrapper around the libxc function to query this.

maybe it should returns an enum libxl_sched_id instead of just a plain int ?

-- 
Vincent

Re: [PATCH 3/4] libxl: add version_info function

[Vincent Hanquez]

On 18/04/10 22:26, Andre Przywara wrote:
> Xen provides a xen_version hypercall to query the values of several
> interesting things (like hypervisor version, commandline used,
> actual changeset, etc.). Create a user-friendly and efficient
> wrapper around the libxc function to provide values for xl info output.

instead of this chain of if else, why don't you just init the structure 
to NULL at the beginning of the call, and not do any else ?

also the query capability seems a bit too much; the info call is 
probably unlikely called into a tight loop, and I expect the number
of call to this to be 1 per starting toolstack or 1 per vm (at worst). 
from an API point of view this would make it simpler too.

last thing, is instead of using strdup, you should use 
libxl_sprintf(ctx, "%s", string) so that you don't have to worry about
freeing memory at all, and you can just omit the free_version_info call 
completely.

-- 
Vincent

Re: [PATCH 1/4] libxl: extend physinfo structure [and 1 more messages]

[Ian Jackson]

Andre Przywara writes ("[Xen-devel] [PATCH 1/4] libxl: extend physinfo structure"):
> The libxl version of the physinfo sysctl does not contain some
> fields like nr_nodes or capabilities needed for xl info output.
> Add them to the structure and the retrieving function.

Rather than this:

  --- a/tools/libxl/libxl.h	Thu Apr 15 19:11:16 2010 +0100
  +++ b/tools/libxl/libxl.h	Sun Apr 18 14:34:32 2010 +0200
  +#define PHYS_CAP_HVM          1
  +#define PHYS_CAP_HVM_DIRECTIO 2

I think it would be better to expect libxl callers to include the
public Xen header file sysctl.h.  While libxl callers should not need
to know about information about the underlying libraris, the Xen
interface is a key public interface and is fine to expose - at least
as far as these kind of bitmasks.

That also avoids the possibility of getting these #defines out of step
with the underlying code (or even the possibility of having to write
conversion code!)

Certainly if you _don't_ convert, you shouldn't redefine the bitfield
constants.

Vincent Hanquez writes ("Re: [Xen-devel] [PATCH 1/4] libxl: extend physinfo structure"):
> I think, It would be much nicer to provide the phys_caps already decoded 
> instead of copying the xc value as is.

Is it really worth reprocessing a bitfield like this which is after
all not actually going to be used much by calling code ?

> so what about something like having phys_cap_hvm and 
> phys_cap_hvm_directio field directly in the structure ? If things 
> extends, we should add new field in libxl anyway.

That's a possibility which I wouldn't object to but it seems
unnecessary.  In that case it should be a series of 1-bit unsigned
C bitfields rather than a set of flags in a uint32.

Ian.

Re: [PATCH 2/4] libxl: add sched_get_id function [and 1 more messages]

[Ian Jackson]

Andre Przywara writes ("[Xen-devel] [PATCH 2/4] libxl: add sched_get_id function"):
> To get the name of the currently used scheduler, Xen provides a sched_id 
> sysctl.
> Add a libxl wrapper around the libxc function to query this.

There should be a doc comemnt in libxl.h saying what the return value
is.  In this case it seems to be one of the values XEN_SCHEDULER_*
from xen/include/public/domctl.h.

Vincent Hanquez writes ("Re: [Xen-devel] [PATCH 2/4] libxl: add sched_get_id function"):
> On 18/04/10 22:25, Andre Przywara wrote:
> > To get the name of the currently used scheduler, Xen provides a sched_id
> > sysctl.
> > Add a libxl wrapper around the libxc function to query this.
> 
> maybe it should returns an enum libxl_sched_id instead of just a plain int ?

That would be an alternative but it does involve extra source code
which TBH I think we can do without.

Ian.

Re: [PATCH 3/4] libxl: add version_info function [and 1 more messages]

[Ian Jackson]

Andre Przywara writes ("[Xen-devel] [PATCH 3/4] libxl: add version_info function"):
> Xen provides a xen_version hypercall to query the values of several
> interesting things (like hypervisor version, commandline used,
> actual changeset, etc.). Create a user-friendly and efficient
> wrapper around the libxc function to provide values for xl info output.

As Vincent says, I think the query mask is overkill.  I would suggeset
just doing without it, and always acquire and return all the
information.  Ie, do away with query_mask and LIBXL_VERSION_FOO_MASK
and the correspondings ifs.


+#define FREE_IF_NOT_ZERO(ptr) if((ptr) != NULL) {free(ptr); ptr = NULL;}

This is redundant because  free(NULL)  is a legal no-op.  It is also
bad macro practice to include an unterminated if like that - it might
eat subsequent elses.  Furthermore, you should wrap ptr up in parens
to avoid macro precedence problems.

So you should write something like one of these:

+#define FREE_AND_ZERO(ptr)  (free((ptr), (ptr) = 0)
+#define FREE_AND_ZERO(ptr)  do{ free((ptr)); (ptr) = 0; }while(0)

Finally, this macro should be in libxl_internal.h where every part of
libxl can use it.


Vincent Hanquez writes ("Re: [Xen-devel] [PATCH 3/4] libxl: add version_info function"):
> instead of this chain of if else, why don't you just init the structure 
> to NULL at the beginning of the call, and not do any else ?

Yes.

Furthermore the documentation comment should make it clear that the
version info structure can be undefined beforehand (and the reader can
therefore conclude that libxl_get_version_info won't free it).

> last thing, is instead of using strdup, you should use 
> libxl_sprintf(ctx, "%s", string) so that you don't have to worry about
> freeing memory at all, and you can just omit the free_version_info call 
> completely.

I don't think that's correct because memory allocated by libxl_sprintf
does not survive return from libxl into the caller.

Ian.

Re: [PATCH 4/4] xl: add "xl info" command

[Ian Jackson]

Andre Przywara writes ("[Xen-devel] [PATCH 4/4] xl: add "xl info" command"):
> The info subcommand was missing from the xl tool. Use the new libxl
> wrapper functions to create a clone of "xm info". The splitting into
> several smaller functions is enspired by the implementation in XendNode.py.
> 
> Signed-off-by: Andre Przywara <andre.przywara@amd.com>

This patch is fine in itself, if you make the edits needed to deal
with the changes we have suggested in your libxl patch.

Thanks for your contribution.

Ian.

Re: [PATCH 3/4] libxl: add version_info function [and 1 more messages]

[Vincent Hanquez]

On 19/04/10 16:36, Ian Jackson wrote:
> I don't think that's correct because memory allocated by libxl_sprintf
> does not survive return from libxl into the caller.

It survives as long as the context is not ctx_free. i.e. if the data 
need to preserve longer, they will have do be duplicated before freeing 
the context.

This is to get away with having to deal with memory leak as much as 
possible and potential more complex code related to errors handling.

-- 
Vincent

Re: [PATCH 3/4] libxl: add version_info function [and 1 more messages]

[Ian Jackson]

Vincent Hanquez writes ("Re: [Xen-devel] [PATCH 3/4] libxl: add version_info function [and 1 more messages]"):
> On 19/04/10 16:36, Ian Jackson wrote:
> > I don't think that's correct because memory allocated by libxl_sprintf
> > does not survive return from libxl into the caller.
> 
> It survives as long as the context is not ctx_free. i.e. if the data 
> need to preserve longer, they will have do be duplicated before freeing 
> the context.

This is sadly not a really tenable behaviour in the long term because
(a) a long-running caller does not want to free the context (and
necessarily reestablish xenstore connections etc. etc.) (b) the caller
may anyway not be able to free the context if doing so frees data they
are still using.

There are not (if I'm not mistaken) currently any uses of the libxl
allocators for data which persists beyond the current libxl entrypoint
and we shouldn't introduce any now.  If there are any they should be
removed.

Ian.

Re: [PATCH 3/4] libxl: add version_info function [and 1 more messages]

[Vincent Hanquez]

On 19/04/10 17:21, Ian Jackson wrote:
> Vincent Hanquez writes ("Re: [Xen-devel] [PATCH 3/4] libxl: add version_info function [and 1 more messages]"):
>> On 19/04/10 16:36, Ian Jackson wrote:
>>> I don't think that's correct because memory allocated by libxl_sprintf
>>> does not survive return from libxl into the caller.
>>
>> It survives as long as the context is not ctx_free. i.e. if the data
>> need to preserve longer, they will have do be duplicated before freeing
>> the context.
>
> This is sadly not a really tenable behaviour in the long term because
> (a) a long-running caller does not want to free the context (and
> necessarily reestablish xenstore connections etc. etc.) (b) the caller
> may anyway not be able to free the context if doing so frees data they
> are still using.

This is not perfect, and has a (tiny) cost, but it makes writing the 
current ocaml bindings (and the future python bindings) much easier and 
much less memory leak prone.

Limiting the number of possible boxes of error type, will definitely 
help getting XCP ported on libxl.

-- 
Vincent

Re: [PATCH 3/4] libxl: add version_info function [and 1 more messages]

[Andre Przywara]

Ian Jackson wrote:
> Andre Przywara writes ("[Xen-devel] [PATCH 3/4] libxl: add version_info function"):
>> Xen provides a xen_version hypercall to query the values of several
>> interesting things (like hypervisor version, commandline used,
>> actual changeset, etc.). Create a user-friendly and efficient
>> wrapper around the libxc function to provide values for xl info output.
> 
> As Vincent says, I think the query mask is overkill.  I would suggeset
> just doing without it, and always acquire and return all the
> information.  Ie, do away with query_mask and LIBXL_VERSION_FOO_MASK
> and the correspondings ifs.
I am not fully convinced of this. There is quite a lot of information 
copied (up to 4KB), and some fields (like pagesize or the version 
numbers) are just plain integers. And this approach just propagates the 
underlying design.
In the xl info implementation I used this feature to just get the 
pagesize. One could think of just querying the version number, too. If 
you don't like the additional parameter, what about a wrapping macro, then?
I don't see the problem with the complexity, though, as it is hidden 
inside the library.
I can use libxl_sprintf function to get rid of the free() function, but 
I personally don't like the idea of accumulated mallocs much, left alone 
the "hidden" free operation.

I will incorporate your other comments.

Thanks for the review!

Regards,
Andre.

-- 
Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
Tel: +49 351 488-3567-12

Re: [PATCH 3/4] libxl: add version_info function [and 1 more messages]

[Vincent Hanquez]

On 19/04/10 21:43, Andre Przywara wrote:
> I am not fully convinced of this. There is quite a lot of information
> copied (up to 4KB), and some fields (like pagesize or the version
> numbers) are just plain integers. And this approach just propagates the
> underlying design.
> In the xl info implementation I used this feature to just get the
> pagesize. One could think of just querying the version number, too. If
> you don't like the additional parameter, what about a wrapping macro, then?
> I don't see the problem with the complexity, though, as it is hidden
> inside the library.

The query mask is exposed to the user, and the structure is partially 
filled. I can imagine that it's going to create those kind of issues, 
where the wrong mask is passed, or change later, and then the field the 
user was expecting would be left null.

The macro would help in the previous case, but I still feel it's 
unnecessary. As a user, I would just not bother, and call the function
always with full mask, until I reach the optimisation phase, and its 
appears on the graph.

A couple of K of data copied shouldn't make a difference, and would be 
completely lost in noise, however if you really think it makes a 
difference, you could have special dedicated info call to get specific 
things like version number.

> I can use libxl_sprintf function to get rid of the free() function, but
> I personally don't like the idea of accumulated mallocs much, left alone
> the "hidden" free operation.

There is a real advantage of permitting a fast development of libxl (and 
its bindings).

This was to get started at the beggining; when we reach a point where 
everything is working we could improve or remove the memory system. But 
I'm fairly convinced it's not going to make a difference for this kind 
of library.

-- 
Vincent

Re: [PATCH 3/4] libxl: add version_info function [and 1 more messages]

[Andre Przywara]

Vincent Hanquez wrote:
> On 19/04/10 21:43, Andre Przywara wrote:
>> I am not fully convinced of this. There is quite a lot of information
>> copied (up to 4KB), and some fields (like pagesize or the version
>> numbers) are just plain integers. And this approach just propagates the
>> underlying design.
>> In the xl info implementation I used this feature to just get the
>> pagesize. One could think of just querying the version number, too. If
>> you don't like the additional parameter, what about a wrapping macro, 
>> then?
>> I don't see the problem with the complexity, though, as it is hidden
>> inside the library.
> 
> The query mask is exposed to the user, and the structure is partially 
> filled. I can imagine that it's going to create those kind of issues, 
> where the wrong mask is passed, or change later, and then the field the 
> user was expecting would be left null.
> 
> The macro would help in the previous case, but I still feel it's 
> unnecessary. As a user, I would just not bother, and call the function
> always with full mask, until I reach the optimisation phase, and its 
> appears on the graph.
> 
> A couple of K of data copied shouldn't make a difference, and would be 
> completely lost in noise, however if you really think it makes a 
> difference, you could have special dedicated info call to get specific 
> things like version number.
I just wanted to avoid introducing a lot of functions which could be 
actually one. And  if a user passes a wrong mask, then well, he is just 
doing the wrong thing and cannot expect it to work. If Xen provides a 
possibility to query single parts of the information, why should we hide 
this capability from the upper layers?
Anyway, my plan was now to make the current mask function static and 
create three exported functions, one for getting all the information, 
one for getting the pagesize and one for getting the Xen version number 
only. Together with a comment one could later expose the mask function 
again if the need arises.
But when I was looking at the libxl_sprintf thing, I realized that all 
the information in the info structure is static and will never change 
during runtime. Am I right? If so, we can solve this whole thing by not 
only storing the strings in libxl_ctx, but the whole structure. The 
first call to the function would query all the members, subsequent calls 
would just return the pointer. This avoids duplication should the 
function be called multiple times. It would be automatically freed when 
destroying the xl context.

What do you think of this?

Regards,
Andre.


-- 
Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
Tel: +49 351 448-3567-12

Re: [PATCH 3/4] libxl: add version_info function [and 1 more messages]

[Vincent Hanquez]

On 21/04/10 13:10, Andre Przywara wrote:
> I just wanted to avoid introducing a lot of functions which could be
> actually one. And  if a user passes a wrong mask, then well, he is just
> doing the wrong thing and cannot expect it to work. If Xen provides a
> possibility to query single parts of the information, why should we hide
> this capability from the upper layers?

well that's just for simplicity.

> But when I was looking at the libxl_sprintf thing, I realized that all
> the information in the info structure is static and will never change
> during runtime. Am I right? If so, we can solve this whole thing by not
> only storing the strings in libxl_ctx, but the whole structure. The
> first call to the function would query all the members, subsequent calls
> would just return the pointer. This avoids duplication should the
> function be called multiple times. It would be automatically freed when
> destroying the xl context.
>
> What do you think of this?

I think that's quite reasonable and a good idea all in all. the API 
stays simple, and most potential performance issues are just dealt with.

-- 
Vincent