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

Re: [Libvir] [PATCH] BZ#251641: Allow to change the cpu pinning for inactive domain



Morning Saori,

You need to call the error function for each error condition, eg:

if (mapstr == NULL) {
xenXMError (domain ? domain->conn : NULL, VIR_ERR_NO_MEMORY, __FUNCTION__);
  return -1;
}

(Much of the current libvirt code gets this wrong, and it's a great deal of pain caused by virterror, but there's no sensible way to change things from here.)

The inner loop is still quadratic even with snprintf because you're copying the string each time. I've changed it below to use a virBuffer instead.

If you ./configure --enable-compile-warnings=error then any warnings will turn into errors. There was one warning in the patch you sent.

Modified version of the patch is attached. **NOTE** I've only compiled it, because I don't have any suitable machine to test it on.

Rich.

PS. Even using the supposedly simplified virBuffer, this is still a good example of why we should not be writing any more code in C. It's not 1985 any more. The language should take care of garbage collection, typing, strings, structures, error handling, etc.

--
Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/
Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod
Street, Windsor, Berkshire, SL4 1TE, United Kingdom.  Registered in
England and Wales under Company Registration No. 03798903
Index: src/xm_internal.c
===================================================================
RCS file: /data/cvs/libvirt/src/xm_internal.c,v
retrieving revision 1.47
diff -u -r1.47 xm_internal.c
--- src/xm_internal.c	21 Nov 2007 16:31:30 -0000	1.47
+++ src/xm_internal.c	27 Nov 2007 13:18:33 -0000
@@ -52,6 +52,9 @@
 #include "buf.h"
 #include "uuid.h"
 
+static int xenXMConfigSetString(virConfPtr conf, const char *setting,
+                                const char *str);
+
 typedef struct xenXMConfCache *xenXMConfCachePtr;
 typedef struct xenXMConfCache {
     time_t refreshedAt;
@@ -101,7 +104,7 @@
     NULL, /* domainRestore */
     NULL, /* domainCoreDump */
     xenXMDomainSetVcpus, /* domainSetVcpus */
-    NULL, /* domainPinVcpu */
+    xenXMDomainPinVcpu, /* domainPinVcpu */
     NULL, /* domainGetVcpus */
     NULL, /* domainGetMaxVcpus */
     xenXMListDefinedDomains, /* listDefinedDomains */
@@ -1215,6 +1218,106 @@
     return (0);
 }
 
+/**
+ * xenXMDomainPinVcpu:
+ * @domain: pointer to domain object
+ * @vcpu: virtual CPU number (reserved)
+ * @cpumap: pointer to a bit map of real CPUs (in 8-bit bytes)
+ * @maplen: length of cpumap in bytes
+ *
+ * Set the vcpu affinity in config
+ *
+ * Returns 0 for success; -1 (with errno) on error
+ */
+int xenXMDomainPinVcpu(virDomainPtr domain,
+                       unsigned int vcpu ATTRIBUTE_UNUSED,
+                       unsigned char *cpumap, int maplen)
+{
+    const char *filename;
+    xenXMConfCachePtr entry;
+    virBufferPtr mapbuf;
+    char *mapstr = NULL;
+    char *ranges = NULL;
+    int i, j, n, comma = 0;
+    int ret = -1;
+
+    if (domain == NULL || domain->conn == NULL || domain->name == NULL
+        || cpumap == NULL || maplen < 1 || maplen > (int)sizeof(cpumap_t)) {
+        xenXMError(domain ? domain->conn : NULL, VIR_ERR_INVALID_ARG,
+                   __FUNCTION__);
+        return -1;
+    }
+    if (domain->conn->flags & VIR_CONNECT_RO) {
+        xenXMError (domain->conn, VIR_ERR_INVALID_ARG, "read only connection");
+        return -1;
+    }
+    if (domain->id != -1) {
+        xenXMError (domain->conn, VIR_ERR_INVALID_ARG, "not inactive domain");
+        return -1;
+    }
+
+    if (!(filename = virHashLookup(nameConfigMap, domain->name))) {
+        xenXMError (domain->conn, VIR_ERR_INTERNAL_ERROR, "virHashLookup");
+        return -1;
+    }
+    if (!(entry = virHashLookup(configCache, filename))) {
+        xenXMError (domain->conn, VIR_ERR_INTERNAL_ERROR,
+                    "can't retrieve config file for domain");
+        return -1;
+    }
+
+    /* from bit map, build character string of mapped CPU numbers */
+    mapbuf = virBufferNew (16);
+    if (mapbuf == NULL) {
+        xenXMError (domain->conn, VIR_ERR_NO_MEMORY, __FUNCTION__);
+        return -1;
+    }
+    for (i = 0; i < maplen; i++)
+        for (j = 0; j < 8; j++)
+            if ((cpumap[i] & (1 << j))) {
+                n = i*8 + j;
+
+                if (comma) {
+                    if (virBufferAdd (mapbuf, ",", 1) == -1) {
+                        xenXMError (domain->conn, VIR_ERR_NO_MEMORY, __FUNCTION__);
+                        virBufferFree (mapbuf);
+                    return -1;
+                    }
+                }
+                comma = 1;
+
+                if (virBufferVSprintf (mapbuf, "%d", n) == -1) {
+                    xenXMError (domain->conn, VIR_ERR_NO_MEMORY, __FUNCTION__);
+                    virBufferFree (mapbuf);
+                    return -1;
+                }
+            }
+
+    mapstr = virBufferContentAndFree (mapbuf);
+
+    /* convert the mapstr to a range based string */
+    ranges = virConvertCpuSet(domain->conn, mapstr, 0);
+
+    if (ranges != NULL) {
+        if (xenXMConfigSetString(entry->conf, "cpus", ranges) < 0)
+            goto cleanup;
+    } else
+        if (xenXMConfigSetString(entry->conf, "cpus", mapstr) < 0)
+            goto cleanup;
+
+    if (virConfWriteFile(entry->filename, entry->conf) < 0)
+        goto cleanup;
+
+    ret = 0;
+
+ cleanup:
+    if(mapstr)
+        free(mapstr);
+    if(ranges)
+        free(ranges);
+    return (ret);
+}
+
 /*
  * Find an inactive domain based on its name
  */
Index: src/xm_internal.h
===================================================================
RCS file: /data/cvs/libvirt/src/xm_internal.h,v
retrieving revision 1.6
diff -u -r1.6 xm_internal.h
--- src/xm_internal.h	14 Nov 2007 11:40:57 -0000	1.6
+++ src/xm_internal.h	27 Nov 2007 13:18:33 -0000
@@ -45,6 +45,8 @@
 int xenXMDomainSetMaxMemory(virDomainPtr domain, unsigned long memory);
 unsigned long xenXMDomainGetMaxMemory(virDomainPtr domain);
 int xenXMDomainSetVcpus(virDomainPtr domain, unsigned int vcpus);
+int xenXMDomainPinVcpu(virDomainPtr domain, unsigned int vcpu,
+                       unsigned char *cpumap, int maplen);
 virDomainPtr xenXMDomainLookupByName(virConnectPtr conn, const char *domname);
 virDomainPtr xenXMDomainLookupByUUID(virConnectPtr conn,
 				     const unsigned char *uuid);

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature


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