[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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/


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]