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/