[PATCH 0 of 6] libxl: support json for pretty printing objects

[PATCH 0 of 6] libxl: support json for pretty printing objects

[Ian Campbell]

Now that Anthony's QMP series is in we can build upon the use of YAJL
to add support for pretty printing libxl objects as JSON.

Also includes a user in xl (to print disks on dry run) and an
associated fix to the check-xl-disk-parse test script.

I've appended a couple of unrelated fixes to the xl disk code, only
because they also touch check-xl-disk-parse.

[PATCH 1 of 6] libxl: IDL: autogenerate functions to produce JSON from libxl data structures

[Ian Campbell]

# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1317312134 -3600
# Node ID 030e844fccebcc3984c5792559c0cbd5424d277a
# Parent  fb42038b1f5cd4f72c5cfff6e5f8ffa46f2fa0b2
libxl: IDL: autogenerate functions to produce JSON from libxl data structures.

Two functions are provided. TYPE_gen_json exposes an interface which is
compatible with the YAGL generator infrastructure. TYPE_to_string uses this to
produce a pretty printed string.

The TYPE_gen_json functions are defined in a new header libxl_json.h which is
not exposed via libxl.h due to the use of YAGL datatypes to avoid poluting the
namespace us libxl users which don't use the library themselves.  If a libxl
user is interested in integrating at the YAGL level then it should #include
this file itself.

Also update testidl to generate a random version of each IDL datastructure and
convert it to JSON. Unfortunately this requires a libxl_ctx and therefore the
test must be run on a Xen system now.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

diff -r fb42038b1f5c -r 030e844fcceb tools/libxl/Makefile
--- a/tools/libxl/Makefile	Thu Sep 29 16:57:52 2011 +0100
+++ b/tools/libxl/Makefile	Thu Sep 29 17:02:14 2011 +0100
@@ -84,14 +84,17 @@ _libxl_paths.h: genpath
 libxl_paths.c: _libxl_paths.h
 
 libxl.h: _libxl_types.h
+libxl_json.h: _libxl_types_json.h
 libxl_internal.h: _libxl_types_internal.h
+libxl_internal_json.h: _libxl_types_internal_json.h
 
 $(LIBXL_OBJS) $(LIBXLU_OBJS) $(XL_OBJS): libxl.h
 $(LIBXL_OBJS): libxl_internal.h
 
-_libxl_type%.h _libxl_type%.c: libxl_type%.idl gentypes.py libxltypes.py
-	$(PYTHON) gentypes.py libxl_type$*.idl __libxl_type$*.h __libxl_type$*.c
+_libxl_type%.h _libxl_type%_json.h _libxl_type%.c: libxl_type%.idl gentypes.py libxltypes.py
+	$(PYTHON) gentypes.py libxl_type$*.idl __libxl_type$*.h __libxl_type$*_json.h __libxl_type$*.c
 	$(call move-if-changed,__libxl_type$*.h,_libxl_type$*.h)
+	$(call move-if-changed,__libxl_type$*_json.h,_libxl_type$*_json.h)
 	$(call move-if-changed,__libxl_type$*.c,_libxl_type$*.c)
 
 libxenlight.so: libxenlight.so.$(MAJOR)
@@ -140,7 +143,7 @@ install: all
 	ln -sf libxlutil.so.$(XLUMAJOR).$(XLUMINOR) $(DESTDIR)$(LIBDIR)/libxlutil.so.$(XLUMAJOR)
 	ln -sf libxlutil.so.$(XLUMAJOR) $(DESTDIR)$(LIBDIR)/libxlutil.so
 	$(INSTALL_DATA) libxlutil.a $(DESTDIR)$(LIBDIR)
-	$(INSTALL_DATA) libxl.h _libxl_types.h libxl_utils.h libxl_uuid.h $(DESTDIR)$(INCLUDEDIR)
+	$(INSTALL_DATA) libxl.h libxl_json.h _libxl_types.h _libxl_types_json.h libxl_utils.h libxl_uuid.h $(DESTDIR)$(INCLUDEDIR)
 	$(INSTALL_DATA) bash-completion $(DESTDIR)$(BASH_COMPLETION_DIR)/xl.sh
 
 .PHONY: clean
diff -r fb42038b1f5c -r 030e844fcceb tools/libxl/gentest.py
--- a/tools/libxl/gentest.py	Thu Sep 29 16:57:52 2011 +0100
+++ b/tools/libxl/gentest.py	Thu Sep 29 17:02:14 2011 +0100
@@ -5,6 +5,7 @@ import re
 import random
 
 import libxltypes
+
 def randomize_char(c):
     if random.random() < 0.5:
         return str.lower(c)
@@ -15,6 +16,50 @@ def randomize_case(s):
     r = [randomize_char(c) for c in s]
     return "".join(r)
 
+def randomize_enum(e):
+    return random.choice([v.name for v in e.values])
+
+handcoded = ["libxl_cpumap", "libxl_key_value_list",
+             "libxl_cpuid_policy_list", "libxl_file_reference",
+             "libxl_string_list", "libxl_cpuarray"]
+
+def gen_rand_init(ty, v, indent = "    ", parent = None):
+    s = ""
+    if isinstance(ty, libxltypes.Enumeration):
+        s += "%s = %s;\n" % (ty.pass_arg(v, parent is None), randomize_enum(ty))
+    elif isinstance(ty, libxltypes.KeyedUnion):
+        if parent is None:
+            raise Exception("KeyedUnion type must have a parent")
+        s += "switch (%s) {\n" % (parent + ty.keyvar_name)
+        for f in ty.fields:
+            (nparent,fexpr) = ty.member(v, f, parent is None)
+            s += "case %s:\n" % f.enumname
+            s += gen_rand_init(f.type, fexpr, indent + "    ", nparent)
+            s += "    break;\n"
+        s += "}\n"
+    elif isinstance(ty, libxltypes.Struct) and (parent is None or ty.json_fn is None):
+        for f in [f for f in ty.fields if not f.const]:
+            (nparent,fexpr) = ty.member(v, f, parent is None)
+            s += gen_rand_init(f.type, fexpr, "", nparent)
+    elif hasattr(ty, "rand_init") and ty.rand_init is not None:
+            s += "%s(%s);\n" % (ty.rand_init, ty.pass_arg(v, isref=parent is None, passby=libxltypes.PASS_BY_REFERENCE))
+    elif ty.typename in ["libxl_uuid", "libxl_mac", "libxl_hwcap"]:
+        s += "rand_bytes((uint8_t *)%s, sizeof(*%s));\n" % (v,v)
+    elif ty.typename in ["libxl_domid"] or isinstance(ty, libxltypes.Number):
+        s += "%s = rand() %% (sizeof(%s)*8);\n" % (ty.pass_arg(v, parent is None), ty.pass_arg(v, parent is None))
+    elif ty.typename in ["bool"]:
+        s += "%s = rand() %% 2;\n" % v
+    elif ty.typename in ["char *"]:
+        s += "%s = rand_str();\n" % v
+    elif ty.typename in handcoded:
+        raise Exception("Gen for handcoded %s" % ty.typename)
+    else:
+        raise Exception("Cannot randomly init %s" % ty.typename)
+
+    if s != "":
+        s = indent + s
+    return s.replace("\n", "\n%s" % indent).rstrip(indent)
+
 if __name__ == '__main__':
     if len(sys.argv) < 3:
         print >>sys.stderr, "Usage: gentest.py <idl> <implementation>"
@@ -23,23 +68,191 @@ if __name__ == '__main__':
     random.seed()
 
     idl = sys.argv[1]
-    (_,types) = libxltypes.parse(idl)
+    (builtins,types) = libxltypes.parse(idl)
 
     impl = sys.argv[2]
     f = open(impl, "w")
     f.write("""
 #include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
 #include \"libxl.h\"
+#include \"libxl_utils.h\"
 
+static char *rand_str(void)
+{
+    int i, sz = rand() % 32;
+    char *s = malloc(sz+1);
+    for (i=0; i<sz; i++)
+        s[i] = 'a' + (rand() % 26);
+    s[i] = '\\0';
+    return s;
+}
+
+static void rand_bytes(uint8_t *p, size_t sz)
+{
+    int i;
+    for (i=0; i<sz; i++)
+        p[i] = rand() % 256;
+        //p[i] = i;
+}
+
+static void libxl_cpumap_rand_init(libxl_cpumap *cpumap)
+{
+    int i;
+    cpumap->size = rand() % 16;
+    cpumap->map = calloc(cpumap->size, sizeof(*cpumap->map));
+    libxl_for_each_cpu(i, *cpumap) {
+        if (rand() % 2)
+            libxl_cpumap_set(cpumap, i);
+        else
+            libxl_cpumap_reset(cpumap, i);
+    }
+}
+
+static void libxl_key_value_list_rand_init(libxl_key_value_list *pkvl)
+{
+    int i, nr_kvp = rand() % 16;
+    libxl_key_value_list kvl = calloc(nr_kvp+1, 2*sizeof(char *));
+
+    for (i = 0; i<2*nr_kvp; i += 2) {
+        kvl[i] = rand_str();
+        if (rand() % 8)
+            kvl[i+1] = rand_str();
+        else
+            kvl[i+1] = NULL;
+    }
+    kvl[i] = NULL;
+    kvl[i+1] = NULL;
+    *pkvl = kvl;
+}
+
+static void libxl_cpuid_policy_list_rand_init(libxl_cpuid_policy_list *pp)
+{
+    int i, nr_policies = rand() % 16;
+    struct {
+        const char *n;
+        int w;
+    } options[] = {
+      /* A random selection from libxl_cpuid_parse_config */
+        {"maxleaf",     32},
+        {"family",       8},
+        {"model",        8},
+        {"stepping",     4},
+        {"localapicid",  8},
+        {"proccount",    8},
+        {"clflush",      8},
+        {"brandid",      8},
+        {"f16c",         1},
+        {"avx",          1},
+        {"osxsave",      1},
+        {"xsave",        1},
+        {"aes",          1},
+        {"popcnt",       1},
+        {"movbe",        1},
+        {"x2apic",       1},
+        {"sse4.2",       1},
+        {"sse4.1",       1},
+        {"dca",          1},
+        {"pdcm",         1},
+        {"procpkg",      6},
+    };
+    const int nr_options = sizeof(options)/sizeof(options[0]);
+    char buf[64];
+    libxl_cpuid_policy_list p = NULL;
+
+    for (i = 0; i < nr_policies; i++) {
+        int opt = rand() % nr_options;
+        int val = rand() % (1<<options[opt].w);
+        snprintf(buf, 64, "%s=%#x", options[opt].n, val);
+        libxl_cpuid_parse_config(&p, buf);
+    }
+    *pp = p;
+}
+
+static void libxl_file_reference_rand_init(libxl_file_reference *p)
+{
+    memset(p, 0, sizeof(*p));
+    if (rand() % 8)
+        p->path = rand_str();
+}
+
+static void libxl_string_list_rand_init(libxl_string_list *p)
+{
+    int i, nr = rand() % 16;
+    libxl_string_list l = calloc(nr+1, sizeof(char *));
+
+    for (i = 0; i<nr; i++) {
+        l[i] = rand_str();
+    }
+    l[i] = NULL;
+    *p = l;
+}
+
+static void libxl_cpuarray_rand_init(libxl_cpuarray *p)
+{
+    int i;
+    /* Up to 16 VCPUs on 32 PCPUS */
+    p->entries = rand() % 16;
+    p->array = calloc(p->entries, sizeof(*p->array));
+    for (i = 0; i < p->entries; i++) {
+        int r = rand() % 32*1.5; /* 2:1 valid:invalid */
+        if (r >= 32)
+            p->array[i] = LIBXL_CPUARRAY_INVALID_ENTRY;
+        else
+            p->array[i] = r;
+    }
+}
+""")
+    for ty in builtins + types:
+        if ty.typename not in handcoded:
+            f.write("static void %s_rand_init(%s);\n" % (ty.typename, ty.make_arg("p", passby=libxltypes.PASS_BY_REFERENCE)))
+            f.write("static void %s_rand_init(%s)\n" % (ty.typename, ty.make_arg("p", passby=libxltypes.PASS_BY_REFERENCE)))
+            f.write("{\n")
+            f.write(gen_rand_init(ty, "p"))
+            f.write("}\n")
+            f.write("\n")
+        ty.rand_init = "%s_rand_init" % ty.typename
+
+    f.write("""
 int main(int argc, char **argv)
 {
 """)
 
-    for ty in [t for t in types if isinstance(t,libxltypes.Enumeration)]:
+    for ty in types:
         f.write("    %s %s_val;\n" % (ty.typename, ty.typename))
-    f.write("    int rc;\n")
-    f.write("\n")
+    f.write("""
+    int rc;
+    char *s;
+    xentoollog_logger_stdiostream *logger;
+    libxl_ctx *ctx;
 
+    logger = xtl_createlogger_stdiostream(stderr, XTL_DETAIL, 0);
+    if (!logger) exit(1);
+
+    if (libxl_ctx_alloc(&ctx, LIBXL_VERSION, (xentoollog_logger*)logger)) {
+        fprintf(stderr, "cannot init xl context\\n");
+        exit(1);
+    }
+""")
+    f.write("    printf(\"Testing TYPE_to_json()\\n\");\n")
+    f.write("    printf(\"----------------------\\n\");\n")
+    f.write("    printf(\"\\n\");\n")
+    for ty in [t for t in types if t.json_fn is not None]:
+        arg = ty.typename + "_val"
+        f.write("    %s_rand_init(%s);\n" % (ty.typename, ty.pass_arg(arg, isref=False, passby=libxltypes.PASS_BY_REFERENCE)))
+        f.write("    s = %s_to_json(ctx, %s);\n" % (ty.typename, ty.pass_arg(arg, isref=False)))
+        f.write("    printf(\"%%s: %%s\\n\", \"%s\", s);\n" % ty.typename)
+        f.write("    if (s == NULL) abort();\n")
+        f.write("    free(s);\n")
+        if ty.destructor_fn is not None:
+            f.write("    %s(&%s_val);\n" % (ty.destructor_fn, ty.typename))
+        f.write("\n")
+
+    f.write("    printf(\"Testing Enumerations\\n\");\n")
+    f.write("    printf(\"--------------------\\n\");\n")
+    f.write("    printf(\"\\n\");\n")
     for ty in [t for t in types if isinstance(t,libxltypes.Enumeration)]:
         f.write("    printf(\"%s -- to string:\\n\");\n" % (ty.typename))
         for v in ty.values:
@@ -47,6 +260,12 @@ int main(int argc, char **argv)
                     (v.valuename, v.name, ty.typename, v.name))
         f.write("\n")
 
+        f.write("    printf(\"%s -- to JSON:\\n\");\n" % (ty.typename))
+        for v in ty.values:
+            f.write("    printf(\"\\t%s = %%d = %%s\", %s, %s_to_json(ctx, %s));\n" %\
+                    (v.valuename, v.name, ty.typename, v.name))
+        f.write("\n")
+
         f.write("    printf(\"%s -- from string:\\n\");\n" % (ty.typename))
         for v in [v.valuename for v in ty.values] + ["AN INVALID VALUE"]:
             n = randomize_case(v)
@@ -58,6 +277,11 @@ int main(int argc, char **argv)
                     (v, n, ty.typename))
         f.write("\n")
 
-    f.write("""return 0;
+    f.write("""
+
+    libxl_ctx_free(ctx);
+    xtl_logger_destroy((xentoollog_logger*)logger);
+
+    return 0;
 }
 """)
diff -r fb42038b1f5c -r 030e844fcceb tools/libxl/gentypes.py
--- a/tools/libxl/gentypes.py	Thu Sep 29 16:57:52 2011 +0100
+++ b/tools/libxl/gentypes.py	Thu Sep 29 17:02:14 2011 +0100
@@ -29,7 +29,6 @@ def libxl_C_instance_of(ty, instancename
 
 def libxl_C_type_define(ty, indent = ""):
     s = ""
-
     if isinstance(ty, libxltypes.Enumeration):
         if ty.comment is not None:
             s += format_comment(0, ty.comment)
@@ -76,7 +75,6 @@ def libxl_C_type_define(ty, indent = "")
     return s.replace("\n", "\n%s" % indent)
 
 def libxl_C_type_destroy(ty, v, indent = "    ", parent = None):
-
     s = ""
     if isinstance(ty, libxltypes.KeyedUnion):
         if parent is None:
@@ -100,6 +98,66 @@ def libxl_C_type_destroy(ty, v, indent =
         s = indent + s
     return s.replace("\n", "\n%s" % indent).rstrip(indent)
 
+def libxl_C_type_gen_json(ty, v, indent = "    ", parent = None):
+    s = ""
+    if parent is None:
+        s += "yajl_gen_status s;\n"
+    if isinstance(ty, libxltypes.Enumeration):
+        s += "{\n"
+        s += "    const char *se = %s_to_string(%s);\n" % (ty.typename, ty.pass_arg(v, parent is None))
+        s += "    if (se)\n"
+        s += "        s = yajl_gen_string(hand, (const unsigned char *)se, strlen(se));\n"
+        s += "    else\n"
+        s += "        s = yajl_gen_null(hand);\n"
+        s += "    if (s != yajl_gen_status_ok)\n"
+        s += "        goto out;\n"
+        s += "}\n"
+    elif isinstance(ty, libxltypes.KeyedUnion):
+        if parent is None:
+            raise Exception("KeyedUnion type must have a parent")
+        s += "switch (%s) {\n" % (parent + ty.keyvar_name)
+        for f in ty.fields:
+            (nparent,fexpr) = ty.member(v, f, parent is None)
+            s += "case %s:\n" % f.enumname
+            s += libxl_C_type_gen_json(f.type, fexpr, indent + "    ", nparent)
+            s += "    break;\n"
+        s += "}\n"
+    elif isinstance(ty, libxltypes.Struct) and (parent is None or ty.json_fn is None):
+        s += "s = yajl_gen_map_open(hand);\n"
+        s += "if (s != yajl_gen_status_ok)\n"
+        s += "    goto out;\n"
+        for f in [f for f in ty.fields if not f.const]:
+            (nparent,fexpr) = ty.member(v, f, parent is None)
+            s += "s = yajl_gen_string(hand, (const unsigned char *)\"%s\", sizeof(\"%s\")-1);\n" % (f.name, f.name)
+            s += "if (s != yajl_gen_status_ok)\n"
+            s += "    goto out;\n"
+            s += libxl_C_type_gen_json(f.type, fexpr, "", nparent)
+        s += "s = yajl_gen_map_close(hand);\n"
+        s += "if (s != yajl_gen_status_ok)\n"
+        s += "    goto out;\n"
+    else:
+        if ty.json_fn is not None:
+            s += "s = %s(hand, %s);\n" % (ty.json_fn, ty.pass_arg(v, parent is None))
+            s += "if (s != yajl_gen_status_ok)\n"
+            s += "    goto out;\n"
+
+    if parent is None:
+        s += "out:\n"
+        s += "return s;\n"
+
+    if s != "":
+        s = indent + s
+    return s.replace("\n", "\n%s" % indent).rstrip(indent)
+
+def libxl_C_type_to_json(ty, v, indent = "    "):
+    s = ""
+    gen = "(libxl__gen_json_callback)&%s_gen_json" % ty.typename
+    s += "return libxl__object_to_json(ctx, \"%s\", %s, (void *)%s);\n" % (ty.typename, gen, ty.pass_arg(v, passby=libxltypes.PASS_BY_REFERENCE))
+
+    if s != "":
+        s = indent + s
+    return s.replace("\n", "\n%s" % indent).rstrip(indent)
+
 def libxl_C_enum_to_string(ty, e, indent = "    "):
     s = ""
     s += "switch(%s) {\n" % e
@@ -138,11 +196,11 @@ def libxl_C_enum_from_string(ty, str, e,
 
 
 if __name__ == '__main__':
-    if len(sys.argv) != 4:
-        print >>sys.stderr, "Usage: gentypes.py <idl> <header> <implementation>"
+    if len(sys.argv) != 5:
+        print >>sys.stderr, "Usage: gentypes.py <idl> <header> <header-json> <implementation>"
         sys.exit(1)
 
-    (_, idl, header, impl) = sys.argv
+    (_, idl, header, header_json, impl) = sys.argv
 
     (_,types) = libxltypes.parse(idl)
 
@@ -167,6 +225,8 @@ if __name__ == '__main__':
         f.write(libxl_C_type_define(ty) + ";\n")
         if ty.destructor_fn is not None:
             f.write("void %s(%s);\n" % (ty.destructor_fn, ty.make_arg("p")))
+        if ty.json_fn is not None:
+            f.write("char *%s_to_json(libxl_ctx *ctx, %s);\n" % (ty.typename, ty.make_arg("p")))
         if isinstance(ty, libxltypes.Enumeration):
             f.write("const char *%s_to_string(%s);\n" % (ty.typename, ty.make_arg("p")))
             f.write("int %s_from_string(const char *s, %s);\n" % (ty.typename, ty.make_arg("e", passby=libxltypes.PASS_BY_REFERENCE)))
@@ -176,6 +236,30 @@ if __name__ == '__main__':
     f.write("""#endif /* %s */\n""" % (header_define))
     f.close()
 
+    print "outputting libxl JSON definitions to %s" % header_json
+
+    f = open(header_json, "w")
+
+    header_json_define = header_json.upper().replace('.','_')
+    f.write("""#ifndef %s
+#define %s
+
+/*
+ * DO NOT EDIT.
+ *
+ * This file is autogenerated by
+ * "%s"
+ */
+
+""" % (header_json_define, header_json_define, " ".join(sys.argv)))
+
+    for ty in [ty for ty in types if ty.json_fn is not None]:
+        f.write("yajl_gen_status %s_gen_json(yajl_gen hand, %s);\n" % (ty.typename, ty.make_arg("p", passby=libxltypes.PASS_BY_REFERENCE)))
+
+    f.write("\n")
+    f.write("""#endif /* %s */\n""" % header_json_define)
+    f.close()
+
     print "outputting libxl type implementations to %s" % impl
 
     f = open(impl, "w")
@@ -222,5 +306,17 @@ if __name__ == '__main__':
         f.write("}\n")
         f.write("\n")
 
+    for ty in [t for t in types if t.json_fn is not None]:
+        f.write("yajl_gen_status %s_gen_json(yajl_gen hand, %s)\n" % (ty.typename, ty.make_arg("p", passby=libxltypes.PASS_BY_REFERENCE)))
+        f.write("{\n")
+        f.write(libxl_C_type_gen_json(ty, "p"))
+        f.write("}\n")
+        f.write("\n")
+
+        f.write("char *%s_to_json(libxl_ctx *ctx, %s)\n" % (ty.typename, ty.make_arg("p")))
+        f.write("{\n")
+        f.write(libxl_C_type_to_json(ty, "p"))
+        f.write("}\n")
+        f.write("\n")
 
     f.close()
diff -r fb42038b1f5c -r 030e844fcceb tools/libxl/idl.txt
--- a/tools/libxl/idl.txt	Thu Sep 29 16:57:52 2011 +0100
+++ b/tools/libxl/idl.txt	Thu Sep 29 17:02:14 2011 +0100
@@ -49,6 +49,15 @@ Type.autogenerate_destructor: (default: 
  Indicates if the above named Type.destructor_fn should be
  autogenerated.
 
+Type.json_fn: (default: typename + "_gen_json" or None if type == None)
+
+ The name of the C function which will generate a YAJL data structure
+ representing this type.
+
+Type.autogenerate_json: (default: True)
+
+ Indicates if the above named Type.json_fn should be autogenerated.
+
 Other simple type-Classes
 -------------------------
 
diff -r fb42038b1f5c -r 030e844fcceb tools/libxl/libxl.h
--- a/tools/libxl/libxl.h	Thu Sep 29 16:57:52 2011 +0100
+++ b/tools/libxl/libxl.h	Thu Sep 29 17:02:14 2011 +0100
@@ -199,10 +199,10 @@ typedef struct {
     int v;
 } libxl_enum_string_table;
 
+typedef struct libxl__ctx libxl_ctx;
+
 #include "_libxl_types.h"
 
-typedef struct libxl__ctx libxl_ctx;
-
 const libxl_version_info* libxl_get_version_info(libxl_ctx *ctx);
 
 typedef struct {
diff -r fb42038b1f5c -r 030e844fcceb tools/libxl/libxl_internal.h
--- a/tools/libxl/libxl_internal.h	Thu Sep 29 16:57:52 2011 +0100
+++ b/tools/libxl/libxl_internal.h	Thu Sep 29 17:02:14 2011 +0100
@@ -35,7 +35,9 @@
 
 #include "flexarray.h"
 #include "libxl_utils.h"
+
 #include "_libxl_types_internal.h"
+#include "libxl_json.h"
 
 #define LIBXL_DESTROY_TIMEOUT 10
 #define LIBXL_DEVICE_MODEL_START_TIMEOUT 10
@@ -337,6 +339,14 @@ _hidden char *libxl__cpupoolid_to_name(l
 _hidden int libxl__enum_from_string(const libxl_enum_string_table *t,
                                     const char *s, int *e);
 
+_hidden yajl_gen_status libxl__yajl_gen_asciiz(yajl_gen hand, const char *str);
+
+_hidden yajl_gen_status libxl__string_gen_json(yajl_gen hand, const char *p);
+
+typedef yajl_gen_status (*libxl__gen_json_callback)(yajl_gen hand, void *);
+_hidden char *libxl__object_to_json(libxl_ctx *ctx, const char *type,
+                                    libxl__gen_json_callback gen, void *p);
+
   /* holds the CPUID response for a single CPUID leaf
    * input contains the value of the EAX and ECX register,
    * and each policy string contains a filter to apply to
diff -r fb42038b1f5c -r 030e844fcceb tools/libxl/libxl_json.c
--- a/tools/libxl/libxl_json.c	Thu Sep 29 16:57:52 2011 +0100
+++ b/tools/libxl/libxl_json.c	Thu Sep 29 17:02:14 2011 +0100
@@ -18,6 +18,7 @@
 #include <yajl/yajl_parse.h>
 #include <yajl/yajl_gen.h>
 
+#include <libxl.h>
 #include "libxl_internal.h"
 
 /* #define DEBUG_ANSWER */
@@ -71,6 +72,207 @@ yajl_gen_status libxl__yajl_gen_asciiz(y
     return yajl_gen_string(hand, (const unsigned char *)str, strlen(str));
 }
 
+/*
+ * YAJL generators for builtin libxl types.
+ */
+yajl_gen_status libxl_uuid_gen_json(yajl_gen hand,
+                                    libxl_uuid *uuid)
+{
+    char buf[LIBXL_UUID_FMTLEN+1];
+    snprintf(buf, sizeof(buf), LIBXL_UUID_FMT, LIBXL_UUID_BYTES((*uuid)));
+    return yajl_gen_string(hand, (const unsigned char *)buf, LIBXL_UUID_FMTLEN);
+}
+
+yajl_gen_status libxl_cpumap_gen_json(yajl_gen hand,
+                                      libxl_cpumap *cpumap)
+{
+    yajl_gen_status s;
+    int i;
+
+    s = yajl_gen_array_open(hand);
+    if (s != yajl_gen_status_ok) goto out;
+
+    libxl_for_each_cpu(i, *cpumap) {
+        if (libxl_cpumap_test(cpumap, i)) {
+            s = yajl_gen_integer(hand, i);
+            if (s != yajl_gen_status_ok) goto out;
+        }
+    }
+    s = yajl_gen_array_close(hand);
+out:
+    return s;
+}
+
+yajl_gen_status libxl_key_value_list_gen_json(yajl_gen hand,
+                                              libxl_key_value_list *pkvl)
+{
+    libxl_key_value_list kvl = *pkvl;
+    yajl_gen_status s;
+    int i;
+
+    s = yajl_gen_map_open(hand);
+    if (s != yajl_gen_status_ok) goto out;
+
+    if (!kvl) goto empty;
+
+    for (i = 0; kvl[i] != NULL; i += 2) {
+        s = libxl__yajl_gen_asciiz(hand, kvl[i]);
+        if (s != yajl_gen_status_ok) goto out;
+        if (kvl[i + 1])
+            s = libxl__yajl_gen_asciiz(hand, kvl[i+1]);
+        else
+            s = yajl_gen_null(hand);
+        if (s != yajl_gen_status_ok) goto out;
+    }
+empty:
+    s = yajl_gen_map_close(hand);
+out:
+    return s;
+}
+
+yajl_gen_status libxl_cpuid_policy_list_gen_json(yajl_gen hand,
+                                libxl_cpuid_policy_list *pcpuid)
+{
+    libxl_cpuid_policy_list cpuid = *pcpuid;
+    yajl_gen_status s;
+    const char *input_names[2] = { "leaf", "subleaf" };
+    const char *policy_names[4] = { "eax", "ebx", "ecx", "edx" };
+    int i, j;
+
+    /*
+     * Aiming for:
+     * [
+     *     { 'leaf':    'val-eax',
+     *       'subleaf': 'val-edx',
+     *       'ebx':     'filter',
+     *       'ecx':     'filter',
+     *       'edx':     'filter' }, ],
+     *     { 'leaf':    'val-eax', ..., 'eax': 'filter', ... },
+     *     ... etc ...
+     * }
+     */
+
+    s = yajl_gen_array_open(hand);
+    if (s != yajl_gen_status_ok) goto out;
+
+    if (cpuid == NULL) goto empty;
+
+    for (i = 0; cpuid[i].input[0] != XEN_CPUID_INPUT_UNUSED; i++) {
+        s = yajl_gen_map_open(hand);
+        if (s != yajl_gen_status_ok) goto out;
+
+        for (j = 0; j < 2; j++) {
+            if (cpuid[i].input[j] != XEN_CPUID_INPUT_UNUSED) {
+                s = libxl__yajl_gen_asciiz(hand, input_names[j]);
+                if (s != yajl_gen_status_ok) goto out;
+                s = yajl_gen_integer(hand, cpuid[i].input[j]);
+                if (s != yajl_gen_status_ok) goto out;
+            }
+        }
+
+        for (j = 0; j < 4; j++) {
+            if (cpuid[i].policy[j] != NULL) {
+                s = libxl__yajl_gen_asciiz(hand, policy_names[j]);
+                if (s != yajl_gen_status_ok) goto out;
+                s = yajl_gen_string(hand,
+                               (const unsigned char *)cpuid[i].policy[j], 32);
+                if (s != yajl_gen_status_ok) goto out;
+            }
+        }
+        s = yajl_gen_map_close(hand);
+        if (s != yajl_gen_status_ok) goto out;
+    }
+
+empty:
+    s = yajl_gen_array_close(hand);
+out:
+    return s;
+}
+
+yajl_gen_status libxl_string_list_gen_json(yajl_gen hand, libxl_string_list *pl)
+{
+    libxl_string_list l = *pl;
+    yajl_gen_status s;
+    int i;
+
+    s = yajl_gen_array_open(hand);
+    if (s != yajl_gen_status_ok) goto out;
+
+    if (!l) goto empty;
+
+    for (i = 0; l[i] != NULL; i++) {
+        s = libxl__yajl_gen_asciiz(hand, l[i]);
+        if (s != yajl_gen_status_ok) goto out;
+    }
+empty:
+    s = yajl_gen_array_close(hand);
+out:
+    return s;
+}
+
+yajl_gen_status libxl_mac_gen_json(yajl_gen hand, libxl_mac *mac)
+{
+    char buf[LIBXL_MAC_FMTLEN+1];
+    snprintf(buf, sizeof(buf), LIBXL_MAC_FMT, LIBXL_MAC_BYTES((*mac)));
+    return yajl_gen_string(hand, (const unsigned char *)buf, LIBXL_MAC_FMTLEN);
+}
+
+yajl_gen_status libxl_hwcap_gen_json(yajl_gen hand,
+                                     libxl_hwcap p)
+{
+    yajl_gen_status s;
+    int i;
+
+    s = yajl_gen_array_open(hand);
+    if (s != yajl_gen_status_ok) goto out;
+
+    for(i=0; i<4; i++) {
+        s = yajl_gen_integer(hand, p[i]);
+        if (s != yajl_gen_status_ok) goto out;
+    }
+    s = yajl_gen_array_close(hand);
+out:
+    return s;
+}
+
+yajl_gen_status libxl_cpuarray_gen_json(yajl_gen hand,
+                                        libxl_cpuarray *cpuarray)
+{
+    yajl_gen_status s;
+    int i;
+
+    s = yajl_gen_array_open(hand);
+    if (s != yajl_gen_status_ok) goto out;
+
+    for(i=0; i<cpuarray->entries; i++) {
+        if (cpuarray->array[i] == LIBXL_CPUARRAY_INVALID_ENTRY)
+            s = yajl_gen_null(hand);
+        else
+            s = yajl_gen_integer(hand, cpuarray->array[i]);
+        if (s != yajl_gen_status_ok) goto out;
+    }
+    s = yajl_gen_array_close(hand);
+out:
+    return s;
+}
+
+yajl_gen_status libxl_file_reference_gen_json(yajl_gen hand,
+                                              libxl_file_reference *p)
+{
+    if (p->path)
+        return libxl__yajl_gen_asciiz(hand, p->path);
+    else
+        return yajl_gen_null(hand);
+}
+
+yajl_gen_status libxl__string_gen_json(yajl_gen hand,
+                                       const char *p)
+{
+    if (p)
+        return libxl__yajl_gen_asciiz(hand, p);
+    else
+        return yajl_gen_null(hand);
+}
 
 /*
  * libxl__json_object helper functions
@@ -558,3 +760,68 @@ libxl__json_object *libxl__json_parse(li
         return NULL;
     }
 }
+
+static const char *yajl_gen_status_to_string(yajl_gen_status s)
+{
+        switch (s) {
+        case yajl_gen_status_ok: abort();
+        case yajl_gen_keys_must_be_strings:
+            return "keys must be strings";
+        case yajl_max_depth_exceeded:
+            return "max depth exceeded";
+        case yajl_gen_in_error_state:
+            return "in error state";
+        case yajl_gen_generation_complete:
+            return "generation complete";
+        case yajl_gen_invalid_number:
+            return "invalid number";
+        case yajl_gen_no_buf:
+            return "no buffer";
+#if 0 /* This is in the docs but not implemented in the version I am running. */
+        case yajl_gen_invalid_string:
+            return "invalid string";
+#endif
+        default:
+            return "unknown error";
+        }
+}
+
+char *libxl__object_to_json(libxl_ctx *ctx, const char *type,
+                            libxl__gen_json_callback gen, void *p)
+{
+    yajl_gen_config conf = { 1, "    " };
+    const unsigned char *buf;
+    char *ret = NULL;
+    unsigned int len = 0;
+    yajl_gen_status s;
+    yajl_gen hand;
+
+    hand = yajl_gen_alloc(&conf, NULL);
+    if (!hand)
+        return NULL;
+
+    s = gen(hand, p);
+    if (s != yajl_gen_status_ok)
+        goto out;
+
+    s = yajl_gen_get_buf(hand, &buf, &len);
+    if (s != yajl_gen_status_ok)
+        goto out;
+    ret = strdup((const char *)buf);
+
+out:
+    yajl_gen_free(hand);
+
+    if (s != yajl_gen_status_ok) {
+        LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
+                   "unable to convert %s to JSON representation. "
+                   "YAJL error code %d: %s", type,
+                   s, yajl_gen_status_to_string(s));
+    } else if (!ret) {
+        LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
+                   "unable to allocate space for to JSON representation of %s",
+                   type);
+    }
+
+    return ret;
+}
diff -r fb42038b1f5c -r 030e844fcceb tools/libxl/libxl_json.h
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/tools/libxl/libxl_json.h	Thu Sep 29 17:02:14 2011 +0100
@@ -0,0 +1,42 @@
+/*
+ * Copyright (C) 2011      Citrix Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License as published
+ * by the Free Software Foundation; version 2.1 only. with the special
+ * exception on linking described in file LICENSE.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Lesser General Public License for more details.
+ */
+
+#ifndef LIBXL_JSON_H
+#define LIBXL_JSON_H
+
+#include <yajl/yajl_gen.h>
+
+#include <_libxl_types_json.h>
+#include <_libxl_types_internal_json.h>
+
+yajl_gen_status libxl_uuid_gen_json(yajl_gen hand,
+                                    libxl_uuid *p);
+yajl_gen_status libxl_cpumap_gen_json(yajl_gen hand,
+                                      libxl_cpumap *p);
+yajl_gen_status libxl_key_value_list_gen_json(yajl_gen hand,
+                                              libxl_key_value_list *p);
+yajl_gen_status libxl_cpuid_policy_list_gen_json(yajl_gen hand,
+                                                 libxl_cpuid_policy_list *p);
+yajl_gen_status libxl_string_list_gen_json(yajl_gen hand,
+                                           libxl_string_list *p);
+yajl_gen_status libxl_mac_gen_json(yajl_gen hand,
+                                   libxl_mac *p);
+yajl_gen_status libxl_hwcap_gen_json(yajl_gen hand,
+                                     libxl_hwcap p);
+yajl_gen_status libxl_cpuarray_gen_json(yajl_gen hand,
+                                        libxl_cpuarray *p);
+yajl_gen_status libxl_file_reference_gen_json(yajl_gen hand,
+                                              libxl_file_reference *p);
+
+#endif /* LIBXL_JSON_H */
diff -r fb42038b1f5c -r 030e844fcceb tools/libxl/libxl_types.idl
--- a/tools/libxl/libxl_types.idl	Thu Sep 29 16:57:52 2011 +0100
+++ b/tools/libxl/libxl_types.idl	Thu Sep 29 17:02:14 2011 +0100
@@ -5,7 +5,7 @@
 
 namespace("libxl_")
 
-libxl_domid = Builtin("domid")
+libxl_domid = Builtin("domid", json_fn = "yajl_gen_integer", autogenerate_json = False)
 libxl_uuid = Builtin("uuid", passby=PASS_BY_REFERENCE)
 libxl_mac = Builtin("mac", passby=PASS_BY_REFERENCE)
 libxl_cpumap = Builtin("cpumap", destructor_fn="libxl_cpumap_destroy", passby=PASS_BY_REFERENCE)
diff -r fb42038b1f5c -r 030e844fcceb tools/libxl/libxltypes.py
--- a/tools/libxl/libxltypes.py	Thu Sep 29 16:57:52 2011 +0100
+++ b/tools/libxl/libxltypes.py	Thu Sep 29 17:02:14 2011 +0100
@@ -50,6 +50,13 @@ class Type(object):
 
         self.autogenerate_destructor = kwargs.setdefault('autogenerate_destructor', True)
 
+        if self.typename is not None:
+            self.json_fn = kwargs.setdefault('json_fn', self.typename + "_gen_json")
+        else:
+            self.json_fn = kwargs.setdefault('json_fn', None)
+
+        self.autogenerate_json = kwargs.setdefault('autogenerate_json', True)
+
     def marshal_in(self):
         return self.dir in [DIR_IN, DIR_BOTH]
     def marshal_out(self):
@@ -83,6 +90,7 @@ class Builtin(Type):
     def __init__(self, typename, **kwargs):
         kwargs.setdefault('destructor_fn', None)
         kwargs.setdefault('autogenerate_destructor', False)
+        kwargs.setdefault('autogenerate_json', False)
         Type.__init__(self, typename, **kwargs)
 
 class Number(Builtin):
@@ -90,6 +98,7 @@ class Number(Builtin):
         kwargs.setdefault('namespace', None)
         kwargs.setdefault('destructor_fn', None)
         kwargs.setdefault('signed', False)
+        kwargs.setdefault('json_fn', "yajl_gen_integer")
         self.signed = kwargs['signed']
         Builtin.__init__(self, ctype, **kwargs)
 
@@ -163,6 +172,8 @@ class Aggregate(Type):
                 comment = None
             else:
                 n,t,const,comment = f
+            if n is None:
+                raise ValueError
             self.fields.append(Field(t,n,const=const,comment=comment))
 
     # Returns a tuple (stem, field-expr)
@@ -220,7 +231,10 @@ class KeyedUnion(Aggregate):
 #
 
 void = Builtin("void *", namespace = None)
-bool = Builtin("bool", namespace = None)
+bool = Builtin("bool", namespace = None,
+               json_fn = "yajl_gen_bool",
+               autogenerate_json = False)
+
 size_t = Number("size_t", namespace = None)
 
 integer = Number("int", namespace = None, signed = True)
@@ -230,7 +244,9 @@ uint16 = UInt(16)
 uint32 = UInt(32)
 uint64 = UInt(64)
 
-string = Builtin("char *", namespace = None, destructor_fn = "free")
+string = Builtin("char *", namespace = None, destructor_fn = "free",
+                 json_fn = "libxl__string_gen_json",
+                 autogenerate_json = False)
 
 class OrderedDict(dict):
     """A dictionary which remembers insertion order.

Re: [PATCH 1 of 6] libxl: IDL: autogenerate functions to produce JSON from libxl data structures

[Ian Jackson]

Ian Campbell writes ("[Xen-devel] [PATCH 1 of 6] libxl: IDL: autogenerate functi> libxl: IDL: autogenerate functions to produce JSON from libxl data structures.
...
> Also update testidl to generate a random version of each IDL datastructure and
> convert it to JSON. Unfortunately this requires a libxl_ctx and therefore the
> test must be run on a Xen system now.

Perhaps we should have a new version of libxl_ctx_alloc which doesn't
attempt to connect to xenstore etc., for these kind of purposes.  This
might turn out to be useful for xl's command line handling too.

Not essential for this patch, though.

> +    elif isinstance(ty, libxltypes.Struct) and (parent is None or ty.json_fn is None):

Do we care about these >80-column lines ?

>      f.write("""
>  #include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +
>  #include \"libxl.h\"
> +#include \"libxl_utils.h\"

AIU Python's quoting rules, these \'s are unnecessary.  And indeed
later we have:

> +      /* A random selection from libxl_cpuid_parse_config */
> +        {"maxleaf",     32},

> +static void rand_bytes(uint8_t *p, size_t sz)
> +{
> +    int i;
> +    for (i=0; i<sz; i++)
> +        p[i] = rand() % 256;
> +        //p[i] = i;

This line is leftover cruft ?

> diff -r fb42038b1f5c -r 030e844fcceb tools/libxl/gentypes.py
> --- a/tools/libxl/gentypes.py	Thu Sep 29 16:57:52 2011 +0100
> +++ b/tools/libxl/gentypes.py	Thu Sep 29 17:02:14 2011 +0100
> @@ -29,7 +29,6 @@ def libxl_C_instance_of(ty, instancename

I haven't reviewed this in detail but I have instead looked at the
output:

> yajl_gen_status libxl_uuid_gen_json(yajl_gen hand,
>                                     libxl_uuid *p);

Are we confident that these functions will never need to take a
libxl_ctx ?

> yajl_gen_status libxl_disk_format_gen_json(yajl_gen hand, libxl_disk_format *p)
> {
>     yajl_gen_status s;
>     {
>         const char *se = libxl_disk_format_to_string(*p);
>         if (se)
>             s = yajl_gen_string(hand, (const unsigned char *)se, strlen(se));
>         else
>             s = yajl_gen_null(hand);
>         if (s != yajl_gen_status_ok)
>             goto out;
>     }
>     out:
>     return s;
> }

There are quite a few functions which all look like this.  Perhaps the
bulk of this function should be a helper function, so you end up with
something like this:

 yajl_gen_status libxl_disk_format_gen_json(yajl_gen hand, libxl_disk_format *p)
 {
    return libxl__yajl_gen_enum(hand, libxl_disk_format_to_string(*p));
 }

That might reduce the size of the compiled code quite a bit.  IME
autogenerated code can get very big if one isn't careful.

> diff -r fb42038b1f5c -r 030e844fcceb tools/libxl/idl.txt
> --- a/tools/libxl/idl.txt	Thu Sep 29 16:57:52 2011 +0100
> +++ b/tools/libxl/idl.txt	Thu Sep 29 17:02:14 2011 +0100
...
> +yajl_gen_status libxl_cpuid_policy_list_gen_json(yajl_gen hand,
> +                                libxl_cpuid_policy_list *pcpuid)
> +{

This rather ad-hoc treatment of cpuid policies isn't ideal I think.
Did we want to try to recast them as some more general aggregate ?

> +    libxl_cpuid_policy_list cpuid = *pcpuid;
> +    yajl_gen_status s;
> +    const char *input_names[2] = { "leaf", "subleaf" };
> +    const char *policy_names[4] = { "eax", "ebx", "ecx", "edx" };
> +    int i, j;
> +
> +    /*
> +     * Aiming for:
> +     * [
> +     *     { 'leaf':    'val-eax',
> +     *       'subleaf': 'val-edx',
> +     *       'ebx':     'filter',
> +     *       'ecx':     'filter',
> +     *       'edx':     'filter' }, ],
> +     *     { 'leaf':    'val-eax', ..., 'eax': 'filter', ... },
> +     *     ... etc ...
> +     * }
> +     */

Mismatched brackets, or confused indenting, in the comment.

> diff -r fb42038b1f5c -r 030e844fcceb tools/libxl/libxl_json.h
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/tools/libxl/libxl_json.h	Thu Sep 29 17:02:14 2011 +0100
...
> +yajl_gen_status libxl_uuid_gen_json(yajl_gen hand,
> +                                    libxl_uuid *p);

Shouldn't these declarations be produced automatically by the IDL
compiler ?  After all it's going to generate calls to these functions.

Ian.

Re: [PATCH 1 of 6] libxl: IDL: autogenerate functions to produce JSON from libxl data structures

[Ian Campbell]

On Thu, 2011-10-06 at 15:55 +0100, Ian Jackson wrote:
> Ian Campbell writes ("[Xen-devel] [PATCH 1 of 6] libxl: IDL: autogenerate functi> libxl: IDL: autogenerate functions to produce JSON from libxl data structures.
> ...
> > Also update testidl to generate a random version of each IDL datastructure and
> > convert it to JSON. Unfortunately this requires a libxl_ctx and therefore the
> > test must be run on a Xen system now.
> 
> Perhaps we should have a new version of libxl_ctx_alloc which doesn't
> attempt to connect to xenstore etc., for these kind of purposes.  This
> might turn out to be useful for xl's command line handling too.
> 
> Not essential for this patch, though.

I considered adding a new flag to enable this behaviour, but then I
realised that libxl_ctx_alloc doesn't have a flag parameter... Perhaps
we should add one?

> 
> > +    elif isinstance(ty, libxltypes.Struct) and (parent is None or ty.json_fn is None):
> 
> Do we care about these >80-column lines ?

I suppose we should. The IDL stuff isn't totally 80-column clean right
now but it is close so I may as well clean it up.

> 
> >      f.write("""
> >  #include <stdio.h>
> > +#include <stdlib.h>
> > +#include <string.h>
> > +
> >  #include \"libxl.h\"
> > +#include \"libxl_utils.h\"
> 
> AIU Python's quoting rules, these \'s are unnecessary.  And indeed
> later we have:

Will fix.

> 
> > +      /* A random selection from libxl_cpuid_parse_config */
> > +        {"maxleaf",     32},
> 
> > +static void rand_bytes(uint8_t *p, size_t sz)
> > +{
> > +    int i;
> > +    for (i=0; i<sz; i++)
> > +        p[i] = rand() % 256;
> > +        //p[i] = i;
> 
> This line is leftover cruft ?

Yes.

> 
> > diff -r fb42038b1f5c -r 030e844fcceb tools/libxl/gentypes.py
> > --- a/tools/libxl/gentypes.py	Thu Sep 29 16:57:52 2011 +0100
> > +++ b/tools/libxl/gentypes.py	Thu Sep 29 17:02:14 2011 +0100
> > @@ -29,7 +29,6 @@ def libxl_C_instance_of(ty, instancename
> 
> I haven't reviewed this in detail but I have instead looked at the
> output:
> 
> > yajl_gen_status libxl_uuid_gen_json(yajl_gen hand,
> >                                     libxl_uuid *p);
> 
> Are we confident that these functions will never need to take a
> libxl_ctx ?

My thinking was that these should be consistent with the interface
provided by the equivalent yajl functions (yajl_gen_integer etc).

Partly because otherwise I need to track which types need a ctx and
which don't.

I can't think of any reason a ctx would be needed, any allocations made
would need to be of the type which could be returned to a caller so they
won't be using the gc.

> 
> > yajl_gen_status libxl_disk_format_gen_json(yajl_gen hand, libxl_disk_format *p)
> > {
> >     yajl_gen_status s;
> >     {
> >         const char *se = libxl_disk_format_to_string(*p);
> >         if (se)
> >             s = yajl_gen_string(hand, (const unsigned char *)se, strlen(se));
> >         else
> >             s = yajl_gen_null(hand);
> >         if (s != yajl_gen_status_ok)
> >             goto out;
> >     }
> >     out:
> >     return s;
> > }
> 
> There are quite a few functions which all look like this.  Perhaps the
> bulk of this function should be a helper function, so you end up with
> something like this:
> 
>  yajl_gen_status libxl_disk_format_gen_json(yajl_gen hand, libxl_disk_format *p)
>  {
>     return libxl__yajl_gen_enum(hand, libxl_disk_format_to_string(*p));
>  }
> 
> That might reduce the size of the compiled code quite a bit.  IME
> autogenerated code can get very big if one isn't careful.

Yes, I'll try this.

> > diff -r fb42038b1f5c -r 030e844fcceb tools/libxl/idl.txt
> > --- a/tools/libxl/idl.txt	Thu Sep 29 16:57:52 2011 +0100
> > +++ b/tools/libxl/idl.txt	Thu Sep 29 17:02:14 2011 +0100
> ...
> > +yajl_gen_status libxl_cpuid_policy_list_gen_json(yajl_gen hand,
> > +                                libxl_cpuid_policy_list *pcpuid)
> > +{
> 
> This rather ad-hoc treatment of cpuid policies isn't ideal I think.
> Did we want to try to recast them as some more general aggregate ?

We have a smallish number of "builtin" types which really ought to be
moved into the IDL, but which are generally complex and need more
thought (other than cpuid we have cpumaps, topology info, hwcaps etc).

The cpuid case is interesting because the libxl_cpuid_policy(_list)
types are actually opaque typedefs of libxl__cpuid_policy and so are not
part of the public IDL. We do now have the internal IDL which could be
used and libxl_cpuid_policy_list_gen_json would become a wrapper of
sorts for libxl__cpuid_policy_gen_json. Even that is more complex than
it sounds since the actual type is currently not using the richer
semantic names like "leaf" and "subleaf" or "e*x" but rather has
input[2] and policy[4].

I'm not especially keen to tackle all that right now though.

> > +    libxl_cpuid_policy_list cpuid = *pcpuid;
> > +    yajl_gen_status s;
> > +    const char *input_names[2] = { "leaf", "subleaf" };
> > +    const char *policy_names[4] = { "eax", "ebx", "ecx", "edx" };
> > +    int i, j;
> > +
> > +    /*
> > +     * Aiming for:
> > +     * [
> > +     *     { 'leaf':    'val-eax',
> > +     *       'subleaf': 'val-edx',
> > +     *       'ebx':     'filter',
> > +     *       'ecx':     'filter',
> > +     *       'edx':     'filter' }, ],
> > +     *     { 'leaf':    'val-eax', ..., 'eax': 'filter', ... },
> > +     *     ... etc ...
> > +     * }
> > +     */
> 
> Mismatched brackets, or confused indenting, in the comment.

Will fix. Once I figure out what I meant...

> > diff -r fb42038b1f5c -r 030e844fcceb tools/libxl/libxl_json.h
> > --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> > +++ b/tools/libxl/libxl_json.h	Thu Sep 29 17:02:14 2011 +0100
> ...
> > +yajl_gen_status libxl_uuid_gen_json(yajl_gen hand,
> > +                                    libxl_uuid *p);
> 
> Shouldn't these declarations be produced automatically by the IDL
> compiler ?  After all it's going to generate calls to these functions.

It's somewhat consistent with how we deal with these "builtin" types in
other places (and simpler in gentypes.py) to declare these by hand. I'll
see if I can make it work though.

Thanks for the review.

Ian.

> 
> Ian.

[PATCH 2 of 6] xl: allow check-xl-disk-parse to run against installed xl as well as build dir

[Ian Campbell]

# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1317312717 -3600
# Node ID 3fb003a5d5367ccdfb969fd786b57ef2e103f289
# Parent  030e844fccebcc3984c5792559c0cbd5424d277a
xl: allow check-xl-disk-parse to run against installed xl as well as build dir

I can't run from the current directory since my build box isn't running Xen so
if ./xl doesn't exist use the installed version on the assumption that I've
copied the script to a test host.

I think running from the build dir needs the blktap2 libraries, so update
LD_LIBRARY_PATH as appropriate.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

diff -r 030e844fcceb -r 3fb003a5d536 tools/libxl/check-xl-disk-parse
--- a/tools/libxl/check-xl-disk-parse	Thu Sep 29 17:02:14 2011 +0100
+++ b/tools/libxl/check-xl-disk-parse	Thu Sep 29 17:11:57 2011 +0100
@@ -2,6 +2,13 @@
 
 set -e
 
+if [ -x ./xl ] ; then
+    export LD_LIBRARY_PATH=.:../libxc:../xenstore:../blktap2/control
+    XL=./xl
+else
+    XL=/usr/sbin/xl
+fi
+
 fprefix=tmp.check-xl-disk-parse
 
 expected () {
@@ -14,8 +21,7 @@ one () {
     expected_rc=$1; shift
     printf "test case %s...\n" "$*"
     set +e
-    LD_LIBRARY_PATH=.:../libxc:../xenstore \
-        ./xl -N block-attach 0 "$@" </dev/null >$fprefix.actual 2>/dev/null
+    ${XL} -N block-attach 0 "$@" </dev/null >$fprefix.actual 2>/dev/null
     actual_rc=$?
     diff -u $fprefix.expected $fprefix.actual
     diff_rc=$?

Re: [PATCH 2 of 6] xl: allow check-xl-disk-parse to run against installed xl as well as build dir

[Ian Jackson]

Ian Campbell writes ("[Xen-devel] [PATCH 2 of 6] xl: allow check-xl-disk-parse to run against installed xl as well as build dir"):
> xl: allow check-xl-disk-parse to run against installed xl as well as build dir

This looks good except:

> +    XL=/usr/sbin/xl

XL=xl surely ?

Ian.

Re: [PATCH 2 of 6] xl: allow check-xl-disk-parse to run against installed xl as well as build dir

[Ian Campbell]

On Thu, 2011-10-06 at 15:57 +0100, Ian Jackson wrote:
> Ian Campbell writes ("[Xen-devel] [PATCH 2 of 6] xl: allow check-xl-disk-parse to run against installed xl as well as build dir"):
> > xl: allow check-xl-disk-parse to run against installed xl as well as build dir
> 
> This looks good except:
> 
> > +    XL=/usr/sbin/xl
> 
> XL=xl surely ?

If you prefer, sure.

Ian.

[PATCH 3 of 6] xl: use libxl_device_disk_to_json to pretty print disk configuration

[Ian Campbell]

# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1317312915 -3600
# Node ID 6c2b62f0452a73811f708f162490cf7c1c247295
# Parent  3fb003a5d5367ccdfb969fd786b57ef2e103f289
xl: use libxl_device_disk_to_json to pretty print disk configuration

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

diff -r 3fb003a5d536 -r 6c2b62f0452a tools/libxl/check-xl-disk-parse
--- a/tools/libxl/check-xl-disk-parse	Thu Sep 29 17:11:57 2011 +0100
+++ b/tools/libxl/check-xl-disk-parse	Thu Sep 29 17:15:15 2011 +0100
@@ -51,15 +51,18 @@ expected </dev/null
 one $e foo
 
 expected <<END
-disk.backend_domid = 0
-disk.pdev_path =     /dev/vg/guest-volume
-disk.vdev =          hda
-disk.backend =       0
-disk.format =        4
-disk.script =        (null)
-disk.removable =     0
-disk.readwrite =     1
-disk.is_cdrom =      0
+disk: {
+    "backend_domid": 0,
+    "pdev_path": "/dev/vg/guest-volume",
+    "vdev": "hda",
+    "backend": "unknown",
+    "format": "raw",
+    "script": null,
+    "removable": 0,
+    "readwrite": 1,
+    "is_cdrom": 0
+}
+
 END
 one 0 /dev/vg/guest-volume,,hda
 one 0 /dev/vg/guest-volume,raw,hda,rw
@@ -68,15 +71,18 @@ one 0  format=raw  vdev=hda  access=rw  
 one 0 raw:/dev/vg/guest-volume,hda,w
 
 expected <<END
-disk.backend_domid = 0
-disk.pdev_path =     /root/image.iso
-disk.vdev =          hdc
-disk.backend =       0
-disk.format =        4
-disk.script =        (null)
-disk.removable =     1
-disk.readwrite =     0
-disk.is_cdrom =      1
+disk: {
+    "backend_domid": 0,
+    "pdev_path": "/root/image.iso",
+    "vdev": "hdc",
+    "backend": "unknown",
+    "format": "raw",
+    "script": null,
+    "removable": 1,
+    "readwrite": 0,
+    "is_cdrom": 1
+}
+
 END
 one 0 /root/image.iso,,hdc,cdrom
 one 0 /root/image.iso,,hdc,,cdrom
diff -r 3fb003a5d536 -r 6c2b62f0452a tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c	Thu Sep 29 17:11:57 2011 +0100
+++ b/tools/libxl/xl_cmdimpl.c	Thu Sep 29 17:15:15 2011 +0100
@@ -4109,17 +4109,9 @@ int main_blockattach(int argc, char **ar
     disk.backend_domid = be_domid;
 
     if (dryrun_only) {
-        /* fixme: this should be generated from the idl */
-        /* fixme: enums (backend, format) should be converted to strings */
-        printf("disk.backend_domid = %"PRIx32"\n", disk.backend_domid);
-        printf("disk.pdev_path =     %s\n",        disk.pdev_path);
-        printf("disk.vdev =          %s\n",        disk.vdev);
-        printf("disk.backend =       %d\n",        disk.backend);
-        printf("disk.format =        %d\n",        disk.format);
-        printf("disk.script =        %s\n",        disk.script);
-        printf("disk.removable =     %d\n",        disk.removable);
-        printf("disk.readwrite =     %d\n",        disk.readwrite);
-        printf("disk.is_cdrom =      %d\n",        disk.is_cdrom);
+        char *json = libxl_device_disk_to_json(ctx, &disk);
+        printf("disk: %s\n", json);
+        free(json);
         if (ferror(stdout) || fflush(stdout)) { perror("stdout"); exit(-1); }
         return 0;
     }

Re: [PATCH 3 of 6] xl: use libxl_device_disk_to_json to pretty print disk configuration

[Ian Jackson]

Ian Campbell writes ("[Xen-devel] [PATCH 3 of 6] xl: use libxl_device_disk_to_js> xl: use libxl_device_disk_to_json to pretty print disk configuration
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

This one looks good to me and I'll apply it as part of the rest of the
series when the time comes.

Ian.

[PATCH 4 of 6] libxl: undo 23728:548b2826293e whitespace cleanup to autogenerated file

[Ian Campbell]

# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1317312932 -3600
# Node ID 6056b382a44fd94ead9523a098855830c400ee54
# Parent  6c2b62f0452a73811f708f162490cf7c1c247295
libxl: undo 23728:548b2826293e whitespace cleanup to autogenerated file

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

diff -r 6c2b62f0452a -r 6056b382a44f tools/libxl/libxlu_disk_l.c
--- a/tools/libxl/libxlu_disk_l.c	Thu Sep 29 17:15:15 2011 +0100
+++ b/tools/libxl/libxlu_disk_l.c	Thu Sep 29 17:15:32 2011 +0100
@@ -34,7 +34,7 @@
 #if defined (__STDC_VERSION__) && __STDC_VERSION__ >= 199901L
 
 /* C99 says to define __STDC_LIMIT_MACROS before including stdint.h,
- * if you want the limit (max/min) macros for int types.
+ * if you want the limit (max/min) macros for int types. 
  */
 #ifndef __STDC_LIMIT_MACROS
 #define __STDC_LIMIT_MACROS 1
@@ -51,7 +51,7 @@ typedef uint32_t flex_uint32_t;
 typedef signed char flex_int8_t;
 typedef short int flex_int16_t;
 typedef int flex_int32_t;
-typedef unsigned char flex_uint8_t;
+typedef unsigned char flex_uint8_t; 
 typedef unsigned short int flex_uint16_t;
 typedef unsigned int flex_uint32_t;
 
@@ -184,7 +184,7 @@ typedef struct yy_buffer_state *YY_BUFFE
 #define EOB_ACT_LAST_MATCH 2
 
     #define YY_LESS_LINENO(n)
-
+    
 /* Return all but the first "n" matched characters back to the input stream. */
 #define yyless(n) \
 	do \
@@ -246,7 +246,7 @@ struct yy_buffer_state
 
     int yy_bs_lineno; /**< The line count. */
     int yy_bs_column; /**< The column count. */
-
+    
 	/* Whether to try to fill the input buffer when we reach the
 	 * end of it.
 	 */
@@ -1869,7 +1869,7 @@ static void xlu__disk_yy_load_buffer_sta
     YY_BUFFER_STATE xlu__disk_yy_create_buffer  (FILE * file, int  size , yyscan_t yyscanner)
 {
 	YY_BUFFER_STATE b;
-
+    
 	b = (YY_BUFFER_STATE) xlu__disk_yyalloc(sizeof( struct yy_buffer_state ) ,yyscanner );
 	if ( ! b )
 		YY_FATAL_ERROR( "out of dynamic memory in xlu__disk_yy_create_buffer()" );
@@ -1913,7 +1913,7 @@ static void xlu__disk_yy_load_buffer_sta
 #ifndef __cplusplus
 extern int isatty (int );
 #endif /* __cplusplus */
-
+    
 /* Initializes or reinitializes a buffer.
  * This function is sometimes called more than once on the same buffer,
  * such as during a xlu__disk_yyrestart() or at EOF.
@@ -1939,7 +1939,7 @@ extern int isatty (int );
     }
 
         b->yy_is_interactive = file ? (isatty( fileno(file) ) > 0) : 0;
-
+    
 	errno = oerrno;
 }
 
@@ -2045,9 +2045,9 @@ static void xlu__disk_yyensure_buffer_st
 								, yyscanner);
 		if ( ! yyg->yy_buffer_stack )
 			YY_FATAL_ERROR( "out of dynamic memory in xlu__disk_yyensure_buffer_stack()" );
-
+								  
 		memset(yyg->yy_buffer_stack, 0, num_to_alloc * sizeof(struct yy_buffer_state*));
-
+				
 		yyg->yy_buffer_stack_max = num_to_alloc;
 		yyg->yy_buffer_stack_top = 0;
 		return;
@@ -2076,12 +2076,12 @@ static void xlu__disk_yyensure_buffer_st
  * @param base the character buffer
  * @param size the size in bytes of the character buffer
  * @param yyscanner The scanner object.
- * @return the newly allocated buffer state object.
+ * @return the newly allocated buffer state object. 
  */
 YY_BUFFER_STATE xlu__disk_yy_scan_buffer  (char * base, yy_size_t  size , yyscan_t yyscanner)
 {
 	YY_BUFFER_STATE b;
-
+    
 	if ( size < 2 ||
 	     base[size-2] != YY_END_OF_BUFFER_CHAR ||
 	     base[size-1] != YY_END_OF_BUFFER_CHAR )
@@ -2117,7 +2117,7 @@ YY_BUFFER_STATE xlu__disk_yy_scan_buffer
  */
 YY_BUFFER_STATE xlu__disk_yy_scan_string (yyconst char * yystr , yyscan_t yyscanner)
 {
-
+    
 	return xlu__disk_yy_scan_bytes(yystr,strlen(yystr) ,yyscanner);
 }
 
@@ -2134,7 +2134,7 @@ YY_BUFFER_STATE xlu__disk_yy_scan_bytes 
 	char *buf;
 	yy_size_t n;
 	int i;
-
+    
 	/* Get memory for full buffer, including space for trailing EOB's. */
 	n = _yybytes_len + 2;
 	buf = (char *) xlu__disk_yyalloc(n ,yyscanner );
@@ -2202,10 +2202,10 @@ YY_EXTRA_TYPE xlu__disk_yyget_extra  (yy
 int xlu__disk_yyget_lineno  (yyscan_t yyscanner)
 {
     struct yyguts_t * yyg = (struct yyguts_t*)yyscanner;
-
+    
         if (! YY_CURRENT_BUFFER)
             return 0;
-
+    
     return yylineno;
 }
 
@@ -2215,10 +2215,10 @@ int xlu__disk_yyget_lineno  (yyscan_t yy
 int xlu__disk_yyget_column  (yyscan_t yyscanner)
 {
     struct yyguts_t * yyg = (struct yyguts_t*)yyscanner;
-
+    
         if (! YY_CURRENT_BUFFER)
             return 0;
-
+    
     return yycolumn;
 }
 
@@ -2279,8 +2279,8 @@ void xlu__disk_yyset_lineno (int  line_n
 
         /* lineno is only valid if an input buffer exists. */
         if (! YY_CURRENT_BUFFER )
-           yy_fatal_error( "xlu__disk_yyset_lineno called with no buffer" , yyscanner);
-
+           yy_fatal_error( "xlu__disk_yyset_lineno called with no buffer" , yyscanner); 
+    
     yylineno = line_number;
 }
 
@@ -2294,8 +2294,8 @@ void xlu__disk_yyset_column (int  column
 
         /* column is only valid if an input buffer exists. */
         if (! YY_CURRENT_BUFFER )
-           yy_fatal_error( "xlu__disk_yyset_column called with no buffer" , yyscanner);
-
+           yy_fatal_error( "xlu__disk_yyset_column called with no buffer" , yyscanner); 
+    
     yycolumn = column_no;
 }
 
@@ -2378,20 +2378,20 @@ int xlu__disk_yylex_init_extra(YY_EXTRA_
         errno = EINVAL;
         return 1;
     }
-
+	
     *ptr_yy_globals = (yyscan_t) xlu__disk_yyalloc ( sizeof( struct yyguts_t ), &dummy_yyguts );
-
+	
     if (*ptr_yy_globals == NULL){
         errno = ENOMEM;
         return 1;
     }
-
+    
     /* By setting to 0xAA, we expose bugs in
     yy_init_globals. Leave at 0x00 for releases. */
     memset(*ptr_yy_globals,0x00,sizeof(struct yyguts_t));
-
+    
     xlu__disk_yyset_extra (yy_user_defined, *ptr_yy_globals);
-
+    
     return yy_init_globals ( *ptr_yy_globals );
 }
 
diff -r 6c2b62f0452a -r 6056b382a44f tools/libxl/libxlu_disk_l.h
--- a/tools/libxl/libxlu_disk_l.h	Thu Sep 29 17:15:15 2011 +0100
+++ b/tools/libxl/libxlu_disk_l.h	Thu Sep 29 17:15:32 2011 +0100
@@ -38,7 +38,7 @@
 #if defined (__STDC_VERSION__) && __STDC_VERSION__ >= 199901L
 
 /* C99 says to define __STDC_LIMIT_MACROS before including stdint.h,
- * if you want the limit (max/min) macros for int types.
+ * if you want the limit (max/min) macros for int types. 
  */
 #ifndef __STDC_LIMIT_MACROS
 #define __STDC_LIMIT_MACROS 1
@@ -55,7 +55,7 @@ typedef uint32_t flex_uint32_t;
 typedef signed char flex_int8_t;
 typedef short int flex_int16_t;
 typedef int flex_int32_t;
-typedef unsigned char flex_uint8_t;
+typedef unsigned char flex_uint8_t; 
 typedef unsigned short int flex_uint16_t;
 typedef unsigned int flex_uint32_t;
 
@@ -193,7 +193,7 @@ struct yy_buffer_state
 
     int yy_bs_lineno; /**< The line count. */
     int yy_bs_column; /**< The column count. */
-
+    
 	/* Whether to try to fill the input buffer when we reach the
 	 * end of it.
 	 */

Re: [PATCH 4 of 6] libxl: undo 23728:548b2826293e whitespace cleanup to autogenerated file

[Ian Jackson]

Ian Campbell writes ("[Xen-devel] [PATCH 4 of 6] libxl: undo 23728:548b2826293e whitespace cleanup to autogenerated file"):
> libxl: undo 23728:548b2826293e whitespace cleanup to autogenerated file
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

There seemed no reason to wait with this one.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>

Ian.

[PATCH 5 of 6] libxlu: correctly parse disk "backendtype" field

[Ian Campbell]

# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1317313034 -3600
# Node ID 50cd0fd187b39a263680d8a4b4f1a8511c7c04ee
# Parent  6056b382a44fd94ead9523a098855830c400ee54
libxlu: correctly parse disk "backendtype" field

Currently it tries to parse the value from the full "backendtype=FOO" string
but really it needs to parse from the equals.

Before:
# xl -N block-attach d32-1 backendtype=phy,vdev=xvdb,access=w,target=/dev/VG/debian-x86_32-1b
command line: config parsing error in disk specification: unknown value for backendtype: near `backendtype=phy' in `backendtype=phy,vdev=xvdb,access=w,target=/dev/VG/debian-x86_32-1b'

After:
# xl -N block-attach d32-1 backendtype=phy,vdev=xvdb,access=w,target=/dev/VG/debian-x86_32-1b
disk: {
    "backend_domid": 0,
    "pdev_path": "/dev/VG/debian-x86_32-1b",
    "vdev": "xvdb",
    "backend": "phy",
    "format": "raw",
    "script": null,
    "removable": 0,
    "readwrite": 1,
    "is_cdrom": 0
}

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

diff -r 6056b382a44f -r 50cd0fd187b3 tools/libxl/check-xl-disk-parse
--- a/tools/libxl/check-xl-disk-parse	Thu Sep 29 17:15:32 2011 +0100
+++ b/tools/libxl/check-xl-disk-parse	Thu Sep 29 17:17:14 2011 +0100
@@ -91,4 +91,20 @@ one 0 "format=raw, vdev=hdc, access=ro, 
 one 0  format=raw  vdev=hdc  access=ro  devtype=cdrom  target=/root/image.iso
 one 0 raw:/root/image.iso,hdc:cdrom,ro
 
+expected <<EOF
+disk: {
+    "backend_domid": 0,
+    "pdev_path": "/dev/vg/guest-volume",
+    "vdev": "xvdb",
+    "backend": "phy",
+    "format": "raw",
+    "script": null,
+    "removable": 0,
+    "readwrite": 1,
+    "is_cdrom": 0
+}
+
+EOF
+one 0 backendtype=phy,vdev=xvdb,access=w,target=/dev/vg/guest-volume
+
 complete
diff -r 6056b382a44f -r 50cd0fd187b3 tools/libxl/libxlu_disk_l.c
--- a/tools/libxl/libxlu_disk_l.c	Thu Sep 29 17:15:32 2011 +0100
+++ b/tools/libxl/libxlu_disk_l.c	Thu Sep 29 17:17:14 2011 +0100
@@ -1261,7 +1261,7 @@ case 8:
 /* rule 8 can match eol */
 YY_RULE_SETUP
 #line 142 "libxlu_disk_l.l"
-{ STRIP(','); setbackendtype(DPC,yytext); }
+{ STRIP(','); setbackendtype(DPC,FROMEQUALS); }
 	YY_BREAK
 case 9:
 /* rule 9 can match eol */
diff -r 6056b382a44f -r 50cd0fd187b3 tools/libxl/libxlu_disk_l.l
--- a/tools/libxl/libxlu_disk_l.l	Thu Sep 29 17:15:32 2011 +0100
+++ b/tools/libxl/libxlu_disk_l.l	Thu Sep 29 17:17:14 2011 +0100
@@ -139,7 +139,7 @@ devtype=disk,?	{ DPC->disk->is_cdrom = 0
 devtype=[^,]*,?	{ xlu__disk_err(DPC,yytext,"unknown value for type"); }
 
 access=[^,]*,?	{ STRIP(','); setaccess(DPC, FROMEQUALS); }
-backendtype=[^,]*? { STRIP(','); setbackendtype(DPC,yytext); }
+backendtype=[^,]*? { STRIP(','); setbackendtype(DPC,FROMEQUALS); }
 
 vdev=[^,]*,?	{ STRIP(','); SAVESTRING("vdev", vdev, FROMEQUALS); }
 script=[^,]*,?	{ STRIP(','); SAVESTRING("script", script, FROMEQUALS); }

Re: [PATCH 5 of 6] libxlu: correctly parse disk "backendtype" field

[Ian Jackson]

Ian Campbell writes ("[Xen-devel] [PATCH 5 of 6] libxlu: correctly parse disk "b> libxlu: correctly parse disk "backendtype" field
> 
> Currently it tries to parse the value from the full "backendtype=FOO" string
> but really it needs to parse from the equals.

I have applioed all of this apart from the test case, which is in the
pre-json format, right away, since it's a bugfix.  Please retain the
test case hunk in your series.

Thanks,
Ian.

[PATCH] libxlu: correctly parse disk "backendtype" field

[Daniel De Graaf]

The expression for backendtype was missing a comma.

--- a/tools/libxl/libxlu_disk_l.l
+++ b/tools/libxl/libxlu_disk_l.l
@@ -139,7 +139,7 @@ devtype=disk,?	{ DPC->disk->is_cdrom = 0; }
 devtype=[^,]*,?	{ xlu__disk_err(DPC,yytext,"unknown value for type"); }
 
 access=[^,]*,?	{ STRIP(','); setaccess(DPC, FROMEQUALS); }
-backendtype=[^,]*? { STRIP(','); setbackendtype(DPC,FROMEQUALS); }
+backendtype=[^,]*,? { STRIP(','); setbackendtype(DPC,FROMEQUALS); }
 
 vdev=[^,]*,?	{ STRIP(','); SAVESTRING("vdev", vdev, FROMEQUALS); }
 script=[^,]*,?	{ STRIP(','); SAVESTRING("script", script, FROMEQUALS); }

Re: [PATCH] libxlu: correctly parse disk "backendtype" field

[Ian Campbell]

On Fri, 2011-10-07 at 21:50 +0100, Daniel De Graaf wrote:
> The expression for backendtype was missing a comma.

This appears correct even though things seem to work without, at least:
xl -N block-attach 0 backendtype=phy,vdev=xvdb,access=w,target=/dev/vg/guest-volume
works as expected. Do you have a test case which fails or did you just
find by code inspection?

Acked-by: Ian Campbell <ian.campbell@citrix.com>

(we need a signed-off-by I suspect)

> 
> --- a/tools/libxl/libxlu_disk_l.l
> +++ b/tools/libxl/libxlu_disk_l.l
> @@ -139,7 +139,7 @@ devtype=disk,?	{ DPC->disk->is_cdrom = 0; }
>  devtype=[^,]*,?	{ xlu__disk_err(DPC,yytext,"unknown value for type"); }
>  
>  access=[^,]*,?	{ STRIP(','); setaccess(DPC, FROMEQUALS); }
> -backendtype=[^,]*? { STRIP(','); setbackendtype(DPC,FROMEQUALS); }
> +backendtype=[^,]*,? { STRIP(','); setbackendtype(DPC,FROMEQUALS); }
>  
>  vdev=[^,]*,?	{ STRIP(','); SAVESTRING("vdev", vdev, FROMEQUALS); }
>  script=[^,]*,?	{ STRIP(','); SAVESTRING("script", script, FROMEQUALS); }

Re: [PATCH] libxlu: correctly parse disk "backendtype" field

[Daniel De Graaf]

On 10/07/2011 05:41 PM, Ian Campbell wrote:
> On Fri, 2011-10-07 at 21:50 +0100, Daniel De Graaf wrote:
>> The expression for backendtype was missing a comma.
> 
> This appears correct even though things seem to work without, at least:
> xl -N block-attach 0 backendtype=phy,vdev=xvdb,access=w,target=/dev/vg/guest-volume
> works as expected. Do you have a test case which fails or did you just
> find by code inspection?
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> (we need a signed-off-by I suspect)
> 

Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>

Found when xl was parsing a configuration file with the disk line:

disk = [ 'backendtype=phy,backend=3,vdev=xvda,access=w,target=/dev/clam/vm1' ]

This is after adding another patch allowing backend domain ID for disk/vif to
be changed in the config, so it may or may not show up on its own; I didn't
test the new syntax until I needed it.

>>
>> --- a/tools/libxl/libxlu_disk_l.l
>> +++ b/tools/libxl/libxlu_disk_l.l
>> @@ -139,7 +139,7 @@ devtype=disk,?	{ DPC->disk->is_cdrom = 0; }
>>  devtype=[^,]*,?	{ xlu__disk_err(DPC,yytext,"unknown value for type"); }
>>  
>>  access=[^,]*,?	{ STRIP(','); setaccess(DPC, FROMEQUALS); }
>> -backendtype=[^,]*? { STRIP(','); setbackendtype(DPC,FROMEQUALS); }
>> +backendtype=[^,]*,? { STRIP(','); setbackendtype(DPC,FROMEQUALS); }
>>  
>>  vdev=[^,]*,?	{ STRIP(','); SAVESTRING("vdev", vdev, FROMEQUALS); }
>>  script=[^,]*,?	{ STRIP(','); SAVESTRING("script", script, FROMEQUALS); }
> 

Re: [PATCH] libxlu: correctly parse disk "backendtype" field

[Ian Jackson]

Daniel De Graaf writes ("Re: [PATCH] libxlu: correctly parse disk "backendtype" field"):
> On 10/07/2011 05:41 PM, Ian Campbell wrote:
> > On Fri, 2011-10-07 at 21:50 +0100, Daniel De Graaf wrote:
> >> The expression for backendtype was missing a comma.

Good catch, thanks.

> > This appears correct even though things seem to work without, at least:

Daniel's fix is correct.  Without the ",?", the "backendtype=..."
match would not eat the trailing comma.  The comma would then be
matched by the positional parameter rule, with effects ranging from
unhelpful to fatal.  Ie, it would parse as if you'd written
   backendtype=<something>,,vdev=<blah>
etc.

> > (we need a signed-off-by I suspect)

Thanks, yes.

> Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>

Ian.

[PATCH 6 of 6] libxl: probe disk backend type in libxl_device_disk_add

[Ian Campbell]

# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1317313222 -3600
# Node ID 966d33c61d0b1dcf8545253b198e185c3757bc65
# Parent  50cd0fd187b39a263680d8a4b4f1a8511c7c04ee
libxl: probe disk backend type in libxl_device_disk_add

Without this "xl block-attach" does not work. On create do_domain_create already
catches this.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

diff -r 50cd0fd187b3 -r 966d33c61d0b tools/libxl/libxl.c
--- a/tools/libxl/libxl.c	Thu Sep 29 17:17:14 2011 +0100
+++ b/tools/libxl/libxl.c	Thu Sep 29 17:20:22 2011 +0100
@@ -926,6 +926,9 @@ int libxl_device_disk_add(libxl_ctx *ctx
     libxl__device device;
     int major, minor, rc;
 
+    rc = libxl__device_disk_set_backend(&gc, disk);
+    if (rc) goto out;
+
     front = flexarray_make(16, 1);
     if (!front) {
         rc = ERROR_NOMEM;