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@xxxxxxxxxx | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list