7:14 a.m.
This patch adds the lxc_conf source files.
--
Best Regards,
Dave Leskovec
IBM Linux Technology Center
Open Virtualization
5 p.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/
6:23 p.m.
Dave Leskovec <dlesko(a)linux.vnet.ibm.com> wrote:
This patch adds the lxc_conf source files.
Looks good.
Please add lxcError to the err_func_re list in Makefile.maint, so that
"make syntax-check" checks all uses for translatable string mark-up.
+#define LXC_MAX_TTY_NAME 32
LXC_MAX_TTY_NAME_LENGTH would be a tiny bit clearer.
Otherwise, one might think this represents the maximum number of TTYs.
+typedef struct __lxc_driver lxc_driver_t;
+struct __lxc_driver {
+ lxc_vm_t *vms;
+ int nactivevms;
+ int ninactivevms;
I find mono-case, no-_-separator names hard to read.
Maybe nActiveVMs and nInactiveVMs instead, since that
seems to be the style here?
+ char* configDir;
+};
+
+/* Types and structs */
+
+/* Inline Functions */
+static inline int lxcIsActiveVM(lxc_vm_t *vm)
That pointer parameter can (and hence should) be "const".
Otherwise, any caller with a const *vm pointer must add a dangerous
const-removing cast just to apply this function without compiler warning.
Please audit the other interfaces below, and make pointer parameters
"const" wherever possible.
For example, at least the "vm" parameter to lxcGenerateXML
looks like it may be made const.
...
+char *lxcGenerateXML(virConnectPtr conn,
+ lxc_driver_t *driver,
+ lxc_vm_t *vm,
+ lxc_vm_def_t *def);
...
+/* Functions */
+void lxcError(virConnectPtr conn, virDomainPtr dom, int code,
+ const char *fmt, ...)
+{
This function is almost identical to at least 3 other <subsys>Error
functions in libvirt. No problem adding it now, but eventually, we
really should factor out the common bits.
+ va_list args;
+ char errorMessage[LXC_MAX_ERROR_LEN];
+ const char *codeErrorMessage;
+
+ if (fmt) {
+ va_start(args, fmt);
+ vsnprintf(errorMessage, LXC_MAX_ERROR_LEN-1, fmt, args);
+ va_end(args);
+ } else {
+ errorMessage[0] = '\0';
+ }
+
+ codeErrorMessage = __virErrorMsg(code, fmt);
+ __virRaiseError(conn, dom, NULL, VIR_FROM_LXC, code, VIR_ERR_ERROR,
+ codeErrorMessage, errorMessage, NULL, 0, 0,
+ codeErrorMessage, errorMessage);
+}
...
+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"))) {
+ lxcError(conn, NULL, VIR_ERR_INTERNAL_ERROR,
+ _("missing filesystem type"));
+ goto error;
+ }
+
+ if (xmlStrEqual(fsType, BAD_CAST "mount") == 0) {
+ lxcError(conn, NULL, VIR_ERR_INTERNAL_ERROR,
+ _("invalid filesystem type"));
It'd be nice to include the offending type name in the diagnostic.
...
+ strLen = xmlStrlen(mountSource);
+ if ((strLen > (PATH_MAX-1)) || (0 == strLen)) {
+ lxcError(conn, NULL, VIR_ERR_INTERNAL_ERROR,
+ _("empty or invalid mount source"));
Just say it's invalid, and include the offending string in the diagnostic:
_("invalid mount source: '%s'"), ...
+ goto error;
+ }
+
...
+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));
Another "it'd be nice" suggestion:
include the directory name in the diagnostic.
+ }
+
+ goto load_complete;
+ }
...
+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;
+ }
+
+ if (unlink(configFile) < 0) {
+ lxcError(conn, NULL, VIR_ERR_INTERNAL_ERROR,
+ _("cannot remove config for %s"), name);
It'd be good to include strerror(errno).
+ return -1;
+ }
...
6027
days inactive
6028
days old
2 comments
3 participants
participants (3)
-
Daniel Veillard -
Dave Leskovec -
Jim Meyering