11 a.m.
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(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/