Re: [Libvir] [RFC] 2 of 4 Linux Container support - lxc_conf files

20 Mar 2008


      On Wed, Mar 19, 2008 at 11:14:31PM -0700, Dave Leskovec wrote:
...
This patch adds the lxc_conf source files.
[...]
+#define LXC_MAX_XML_LENGTH 4096
maybe up that a bit, or even better try to avoid a fixed size buffer
but that can be done separately
...
+#define LXC_MAX_ERROR_LEN 1024
+#define LXC_DOMAIN_TYPE "linuxcontainer"
In general try to provide comments for types, even if it looks easy to
figure out :-)
...
+typedef struct __lxc_mount lxc_mount_t;
+struct __lxc_mount {
+    char source[PATH_MAX]; /* user's directory */
+    char target[PATH_MAX];
+
+    lxc_mount_t *next;
+};
+
+typedef struct __lxc_vm_def lxc_vm_def_t;
+struct __lxc_vm_def {
+    unsigned char uuid[VIR_UUID_BUFLEN];
+    char* name;
+    int id;
+
+    /* init command string */
+    char init[PATH_MAX];
+
+    int maxMemory;
In kbytes I assume, maybe an unsigned long would be in order there...
...
+    /* mounts - list of mount structs */
+    int nmounts;
+    lxc_mount_t *mounts;
+
+    /* tty device */
+    char tty[LXC_MAX_TTY_NAME];
+};
[...]
...
Index: b/src/lxc_conf.c
[...]
+static inline int lxcIsEmptyXPathStringObj(xmlXPathObjectPtr xpathObj)
+{
+    if ((xpathObj == NULL) ||
+        (xpathObj->type != XPATH_STRING) ||
+        (xpathObj->stringval == NULL) ||
+        (xpathObj->stringval[0] == 0)) {
+        return 1;
+    } else {
+        return 0;
+    }
+}
best made a convenience function exported from xml.h . I'm not sure 
inlining functions is really a net gain in a libvirt context, it's not
like anything is time critical, but this can add up on the size of the
library quicker than one would expect.
...
+static int lxcParseMountXML(virConnectPtr conn, xmlNodePtr nodePtr,
+                            lxc_mount_t *lxcMount)
+{
+    xmlChar *fsType = NULL;
+    xmlNodePtr curNode;
+    xmlChar *mountSource = NULL;
+    xmlChar *mountTarget = NULL;
+    int strLen;
+    int rc = -1;
+
+    if (NULL == (fsType = xmlGetProp(nodePtr, BAD_CAST "type"))) {
stylistic, separate the affectation and the test, more readable IMHO.
And it makes more obvious the fact that the returned value need to be freed.
...
+        lxcError(conn, NULL, VIR_ERR_INTERNAL_ERROR,
+                 _("missing filesystem type"));
+        goto error;
+    }
[...]
+static int lxcParseDomainName(virConnectPtr conn, char **name,
+                              xmlXPathContextPtr contextPtr)
+{
+    int rc = -1;
+    xmlXPathObjectPtr xpathObj = NULL;
+
+    xpathObj = xmlXPathEval(BAD_CAST "string(/domain/name[1])", contextPtr);
+    if (lxcIsEmptyXPathStringObj(xpathObj)) {
+        lxcError(conn, NULL, VIR_ERR_NO_NAME, NULL);
+        goto parse_complete;
+    }
Hum, why not use virXPathString() it hides the libxml2 XPath details,
...
+    *name = strdup((const char *)xpathObj->stringval);
+    if (NULL == *name) {
+        lxcError(conn, NULL, VIR_ERR_NO_MEMORY, NULL);
+        goto parse_complete;
+    }
+
+
+    rc = 0;
+
+parse_complete:
+    xmlXPathFreeObject(xpathObj);
+    return rc;
+}
and simplifies the cleanup/exit, no strdup, no xmlXPathFreeObject,
I guess each time you use the combo xmlXPathEval/lxcIsEmptyXPathStringObj
a virXPathString call would be clearer and more in line with other libvirt
code
...
+static int lxcParseDomainMounts(virConnectPtr conn,
+                                lxc_mount_t **mounts,
+                                xmlXPathContextPtr contextPtr)
+{
+    int rc = -1;
+    xmlXPathObjectPtr xpathObj = NULL;
+    int i;
+    lxc_mount_t *mountObj;
+    lxc_mount_t *prevObj = NULL;
+    int nmounts = 0;
+
+    xpathObj = xmlXPathEval(BAD_CAST "/domain/devices/filesystem",
Hum, I'm sure using virXPathNodeSet() would make this code clearer
...
+    if ((xpathObj != NULL) &&
+        (xpathObj->type == XPATH_NODESET) &&
+        (xpathObj->nodesetval != NULL) &&
+        (xpathObj->nodesetval->nodeNr >= 0)) {
+        for (i = 0; i < xpathObj->nodesetval->nodeNr; ++i) {
+            mountObj = calloc(1, sizeof(lxc_mount_t));
+            if (NULL == mountObj) {
+                lxcError(conn, NULL, VIR_ERR_NO_MEMORY, "mount");
+                goto parse_complete;
+            }
+
+            rc = lxcParseMountXML(conn, xpathObj->nodesetval->nodeTab[i],
+                                  mountObj);
+            if (0 > rc) {
+                free(mountObj);
+                goto parse_complete;
+            }
+
+            /* set the linked list pointers */
+            nmounts++;
+            mountObj->next = NULL;
+            if (0 == i) {
+                *mounts = mountObj;
+            } else {
+                prevObj->next = mountObj;
+            }
+            prevObj = mountObj;
+        }
+    }
+
+    rc = nmounts;
+
+parse_complete:
+    xmlXPathFreeObject(xpathObj);
+
+    return rc;
+}
[...]
+static int lxcParseDomainMemory(virConnectPtr conn, int* memory, xmlXPathContextPtr contextPtr)
+{
+    int rc = -1;
+    xmlXPathObjectPtr xpathObj = NULL;
+    char *endChar = NULL;
+
+    xpathObj = xmlXPathEval(BAD_CAST "string(/domain/memory[1])", contextPtr);
+    if (lxcIsEmptyXPathStringObj(xpathObj)) {
+        /* not an error, default to an invalid value so it's not used */
+        *memory = -1;
+    } else {
+        *memory = strtoll((const char*)xpathObj->stringval,
+                                          &endChar, 10);
+        if ((endChar == (const char*)xpathObj->stringval) ||
+           (*endChar != '\0')) {
+            lxcError(conn, NULL, VIR_ERR_INTERNAL_ERROR,
+                     _("invalid memory value"));
+            goto parse_complete;
+        }
+    }
use virXPathLong() and keep as a long in the structure,
...
+    rc = 0;
[...]
+static lxc_vm_def_t * lxcParseXML(virConnectPtr conn, xmlDocPtr docPtr)
+{
+    xmlNodePtr rootNodePtr = NULL;
+    xmlXPathContextPtr contextPtr = NULL;
+    xmlChar *xmlProp = NULL;
+    lxc_vm_def_t *containerDef;
+
+    if (!(containerDef = calloc(1, sizeof(*containerDef)))) {
+        lxcError(conn, NULL, VIR_ERR_NO_MEMORY, "containerDef");
+        return NULL;
+    }
+
+    /* Prepare parser / xpath context */
+    rootNodePtr = xmlDocGetRootElement(docPtr);
+    if ((rootNodePtr == NULL) ||
+       (!xmlStrEqual(rootNodePtr->name, BAD_CAST "domain"))) {
+        lxcError(conn, NULL, VIR_ERR_INTERNAL_ERROR,
+                 _("invalid root element"));
+        goto error;
+    }
+
+    contextPtr = xmlXPathNewContext(docPtr);
+    if (contextPtr == NULL) {
+        lxcError(conn, NULL, VIR_ERR_NO_MEMORY, "context");
+        goto error;
+    }
+
+    /* Verify the domain type is linuxcontainer */
+    if (!(xmlProp = xmlGetProp(rootNodePtr, BAD_CAST "type"))) {
+        lxcError(conn, NULL, VIR_ERR_INTERNAL_ERROR,
+                 _("missing domain type"));
+        goto error;
+    }
+
+    if (!(xmlStrEqual(xmlProp, BAD_CAST LXC_DOMAIN_TYPE))) {
+        lxcError(conn, NULL, VIR_ERR_INTERNAL_ERROR,
+                 _("invalid domain type"));
+        goto error;
+    }
+    free(xmlProp);
+    xmlProp = NULL;
Hum, maybe this check could be unified as a simple 
     type = virXPathString("/domain/@type", contextPtr);
and check the returned string against LXC_DOMAIN_TYPE, smaller, simpler
and would insure that the root element is named domain (without namespace)
...
+    if ((xmlProp = xmlGetProp(rootNodePtr, BAD_CAST "id"))) {
+        if (0 > virStrToLong_i((char*)xmlProp, NULL, 10, &(containerDef->id))) {
+            lxcError(conn, NULL, VIR_ERR_INTERNAL_ERROR,
+                     "invalid domain id");
+            goto error;
+        }
+    } else {
+        containerDef->id = -1;
+    }
+    free(xmlProp);
+    xmlProp = NULL;
Same with virXPathString("/domain/@id, ...)
...
+    if (lxcParseDomainName(conn, &(containerDef->name), contextPtr) < 0) {
[...]
+int lxcLoadDriverConfig(virConnectPtr conn)
+{
+    lxc_driver_t *driverPtr = (lxc_driver_t*)conn->privateData;
+
+    /* Set the container configuration directory */
+    driverPtr->configDir = strdup(SYSCONF_DIR "/libvirt/lxc");
+    if (NULL == driverPtr->configDir) {
+        lxcError(conn, NULL, VIR_ERR_NO_MEMORY, "configDir");
+        return -1;
+    }
+
+    return 0;
+}
Hum, is there something missing in that function ?

[...]
[...]
...
+int lxcLoadContainerInfo(virConnectPtr conn)
+{
+    int rc = -1;
+    lxc_driver_t *driverPtr = (lxc_driver_t*)conn->privateData;
+    DIR *dir;
+    struct dirent *dirEntry;
+
+    if (!(dir = opendir(driverPtr->configDir))) {
+        if (ENOENT == errno) {
+            /* no config dir => no containers */
+            rc = 0;
+        } else {
+            lxcError(conn, NULL, VIR_ERR_INTERNAL_ERROR,
+                     _("failed to open config directory: %s"), strerror(errno));
+        }
+
+        goto load_complete;
+    }
+
+    while ((dirEntry = readdir(dir))) {
+        if (dirEntry->d_name[0] == '.') {
+            continue;
+        }
+
+        if (!virFileHasSuffix(dirEntry->d_name, ".xml")) {
+            continue;
+        }
so container definition files are detected by a .xml suffix, maybe
using something like .lxc would be more specific, not a big deal, but
once it's set it's a bit annoying to change

[...]
...
+int lxcDeleteConfig(virConnectPtr conn,
+                    lxc_driver_t *driver ATTRIBUTE_UNUSED,
+                    const char *configFile,
+                    const char *name)
+{
+    if (!configFile[0]) {
+        lxcError(conn, NULL, VIR_ERR_INTERNAL_ERROR,
+                 _("no config file for %s"), name);
+        return -1;
+    }
Hum ... can we make a little bit of checking here for the file
to be an absolute path and having the right prefix ?
...
+    if (unlink(configFile) < 0) {
+        lxcError(conn, NULL, VIR_ERR_INTERNAL_ERROR,
+                 _("cannot remove config for %s"), name);
+        return -1;
+    }
+
+    return 0;
+}
Still look fine, I could make the XPath access cleanups in a second
step.

Daniel

-- 
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard      | virtualization library  http://libvirt.org/
veillard@redhat.com  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine  http://rpmfind.net/