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

Re: [libvirt] [PATCH] esx: Map the .vmx annotation to the domain XML description



2010/8/27 Eric Blake <eblake redhat com>:
> On 08/27/2010 10:31 AM, Matthias Bolte wrote:
>>
>> +    char *annotation = NULL;
>> +    char *tmp;
>> +    int length;
>
> s/int/size_t/
>
>>      int controller;
>>      int bus;
>>      int port;
>> @@ -947,6 +950,33 @@ esxVMX_ParseConfig(esxVMX_Context *ctx, virCapsPtr
>> caps, const char *vmx,
>>          goto cleanup;
>>      }
>>
>> +    /* vmx:annotation ->  def:description */
>> +    if (esxUtil_GetConfigString(conf, "annotation",&def->description,
>> +                                true)<  0) {
>> +        goto cleanup;
>> +    }
>> +
>> +    /* Replace '|22' with '"' and '|7C' with '|' */
>
> Looking at that comment, it looks like |xx for any two arbitrary hex digits
> is a valid escape sequence.  While you only need to be strict in what you
> generate (escapes only for the problematic characters that can't be sent
> literally), should you be liberal in what you accept (all possible | escapes
> for any byte, even if the byte could have been sent literally)?
>
>> +    if (def->description != NULL) {
>> +        length = strlen(def->description) + 1;
>> +        tmp = def->description;
>> +
>> +        while (*tmp != '\0') {
>> +            if (STRPREFIX(tmp, "|22")) {
>> +                *tmp = '"';
>> +                memmove(tmp + 1, tmp + 3, length - 3);
>> +                length -= 2;
>
> Hmm - this scales quadratically (that is, decoding a sequence of 20 "|22"
> requires 19 memmoves, totaling 190 sequence relocations).  I would prefer a
> linear rewrite - have two pointers, one that tracks where you are reading,
> and one that tracks where you are writing (you already did this on the loop
> adding the escapes).  Then, on every iteration, the read pointer may advance
> multiple positions, but the write pointer advances only one, and you don't
> need any memmoves.

In v2 (attached) I made this unescape all |XX and removed the memmoves.

To convert XX to the actual char I moved hextobin (used internally by
virUUIDParse) to util.c and used it here. The move is done in a
separate patch (attached).

Matthias
From e1702de3def2f672a5cdddd47d0141fbb0a809e3 Mon Sep 17 00:00:00 2001
From: Matthias Bolte <matthias bolte googlemail com>
Date: Fri, 27 Aug 2010 23:13:45 +0200
Subject: [PATCH] Move hextobin as virHexToBin to util.c

virHexToBin will be used in the .vmx handling code.
---
 src/util/util.c |   15 +++++++++++++++
 src/util/util.h |    2 ++
 src/util/uuid.c |   20 ++------------------
 3 files changed, 19 insertions(+), 18 deletions(-)

diff --git a/src/util/util.c b/src/util/util.c
index 9679dc9..b6b9712 100644
--- a/src/util/util.c
+++ b/src/util/util.c
@@ -2089,6 +2089,21 @@ virStrToDouble(char const *s,
     return 0;
 }
 
+/* Convert C from hexadecimal character to integer.  */
+int
+virHexToBin(unsigned char c)
+{
+    switch (c) {
+    default: return c - '0';
+    case 'a': case 'A': return 10;
+    case 'b': case 'B': return 11;
+    case 'c': case 'C': return 12;
+    case 'd': case 'D': return 13;
+    case 'e': case 'E': return 14;
+    case 'f': case 'F': return 15;
+    }
+}
+
 /**
  * virSkipSpaces:
  * @str: pointer to the char pointer used
diff --git a/src/util/util.h b/src/util/util.h
index 476eac4..5de4fd6 100644
--- a/src/util/util.h
+++ b/src/util/util.h
@@ -194,6 +194,8 @@ int virStrToDouble(char const *s,
                    char **end_ptr,
                    double *result);
 
+int virHexToBin(unsigned char c);
+
 int virMacAddrCompare (const char *mac1, const char *mac2);
 
 void virSkipSpaces(const char **str);
diff --git a/src/util/uuid.c b/src/util/uuid.c
index 9cafc2a..969ac6d 100644
--- a/src/util/uuid.c
+++ b/src/util/uuid.c
@@ -113,22 +113,6 @@ virUUIDGenerate(unsigned char *uuid)
     return(err);
 }
 
-/* Convert C from hexadecimal character to integer.  */
-static int
-hextobin (unsigned char c)
-{
-  switch (c)
-    {
-    default: return c - '0';
-    case 'a': case 'A': return 10;
-    case 'b': case 'B': return 11;
-    case 'c': case 'C': return 12;
-    case 'd': case 'D': return 13;
-    case 'e': case 'E': return 14;
-    case 'f': case 'F': return 15;
-    }
-}
-
 /**
  * virUUIDParse:
  * @uuidstr: zero terminated string representation of the UUID
@@ -166,14 +150,14 @@ virUUIDParse(const char *uuidstr, unsigned char *uuid) {
         }
         if (!c_isxdigit(*cur))
             goto error;
-        uuid[i] = hextobin(*cur);
+        uuid[i] = virHexToBin(*cur);
         uuid[i] *= 16;
         cur++;
         if (*cur == 0)
             goto error;
         if (!c_isxdigit(*cur))
             goto error;
-        uuid[i] += hextobin(*cur);
+        uuid[i] += virHexToBin(*cur);
         i++;
         cur++;
     }
-- 
1.7.0.4

From fa92d3b46f88f9b1949780110f15bcfa72fd6c2c Mon Sep 17 00:00:00 2001
From: Matthias Bolte <matthias bolte googlemail com>
Date: Fri, 27 Aug 2010 17:23:49 +0200
Subject: [PATCH] esx: Map the .vmx annotation to the domain XML description

Take care of escaping '"' and '|' (the escape character).

Add tests for this.
---
 src/esx/esx_vmx.c                        |  125 ++++++++++++++++++++++++-----
 tests/vmx2xmldata/vmx2xml-annotation.vmx |    3 +
 tests/vmx2xmldata/vmx2xml-annotation.xml |   16 ++++
 tests/vmx2xmltest.c                      |    2 +
 tests/xml2vmxdata/xml2vmx-annotation.vmx |   10 +++
 tests/xml2vmxdata/xml2vmx-annotation.xml |    9 ++
 tests/xml2vmxtest.c                      |    2 +
 7 files changed, 145 insertions(+), 22 deletions(-)
 create mode 100644 tests/vmx2xmldata/vmx2xml-annotation.vmx
 create mode 100644 tests/vmx2xmldata/vmx2xml-annotation.xml
 create mode 100644 tests/xml2vmxdata/xml2vmx-annotation.vmx
 create mode 100644 tests/xml2vmxdata/xml2vmx-annotation.xml

diff --git a/src/esx/esx_vmx.c b/src/esx/esx_vmx.c
index 12cd005..59eb3b2 100644
--- a/src/esx/esx_vmx.c
+++ b/src/esx/esx_vmx.c
@@ -843,6 +843,8 @@ esxVMX_ParseConfig(esxVMX_Context *ctx, virCapsPtr caps, const char *vmx,
     long long numvcpus = 0;
     char *sched_cpu_affinity = NULL;
     char *guestOS = NULL;
+    char *tmp1;
+    char *tmp2;
     int controller;
     int bus;
     int port;
@@ -947,6 +949,36 @@ esxVMX_ParseConfig(esxVMX_Context *ctx, virCapsPtr caps, const char *vmx,
         goto cleanup;
     }
 
+    /* vmx:annotation -> def:description */
+    if (esxUtil_GetConfigString(conf, "annotation", &def->description,
+                                true) < 0) {
+        goto cleanup;
+    }
+
+    /* Unescape '|XX' where X is a hex digit */
+    if (def->description != NULL) {
+        tmp1 = def->description; /* reading from this one */
+        tmp2 = def->description; /* writing to this one */
+
+        while (*tmp1 != '\0') {
+            if (*tmp1 == '|') {
+                if (!c_isxdigit(tmp1[1]) || !c_isxdigit(tmp1[2])) {
+                    ESX_ERROR(VIR_ERR_INTERNAL_ERROR, "%s",
+                              _("VMX entry 'annotation' contains invalid "
+                                "escape sequence"));
+                    goto cleanup;
+                }
+
+                *tmp2++ = virHexToBin(tmp1[1]) * 16 + virHexToBin(tmp1[2]);
+                tmp1 += 3;
+            } else {
+                *tmp2++ = *tmp1++;
+            }
+        }
+
+        *tmp2 = '\0';
+    }
+
     /* vmx:memsize -> def:maxmem */
     if (esxUtil_GetConfigLong(conf, "memsize", &memsize, 32, true) < 0) {
         goto cleanup;
@@ -2314,10 +2346,15 @@ char *
 esxVMX_FormatConfig(esxVMX_Context *ctx, virCapsPtr caps, virDomainDefPtr def,
                     esxVI_ProductVersion productVersion)
 {
+    char *vmx = NULL;
     int i;
     int sched_cpu_affinity_length;
     unsigned char zero[VIR_UUID_BUFLEN];
     virBuffer buffer = VIR_BUFFER_INITIALIZER;
+    size_t length;
+    char *tmp1;
+    char *tmp2;
+    char *annotation = NULL;
     bool scsi_present[4] = { false, false, false, false };
     int scsi_virtualDev[4] = { -1, -1, -1, -1 };
     bool floppy_present[2] = { false, false };
@@ -2361,7 +2398,7 @@ esxVMX_FormatConfig(esxVMX_Context *ctx, virCapsPtr caps, virDomainDefPtr def,
       default:
         ESX_ERROR(VIR_ERR_INTERNAL_ERROR, "%s",
                   _("Unexpected product version"));
-        goto failure;
+        goto cleanup;
     }
 
     /* def:arch -> vmx:guestOS */
@@ -2373,7 +2410,7 @@ esxVMX_FormatConfig(esxVMX_Context *ctx, virCapsPtr caps, virDomainDefPtr def,
         ESX_ERROR(VIR_ERR_INTERNAL_ERROR,
                   _("Expecting domain XML attribute 'arch' of entry 'os/type' "
                     "to be 'i686' or 'x86_64' but found '%s'"), def->os.arch);
-        goto failure;
+        goto cleanup;
     }
 
     /* def:uuid -> vmx:uuid.action, vmx:uuid.bios */
@@ -2392,13 +2429,53 @@ esxVMX_FormatConfig(esxVMX_Context *ctx, virCapsPtr caps, virDomainDefPtr def,
     /* def:name -> vmx:displayName */
     virBufferVSprintf(&buffer, "displayName = \"%s\"\n", def->name);
 
+    /* def:description -> vmx:annotation */
+    if (def->description != NULL) {
+        /* Escape '"' as '|22' and '|' as '|7C' */
+        length = 1; /* 1 byte for termination */
+        tmp1 = def->description;
+
+        while (*tmp1 != '\0') {
+            if (*tmp1 == '"' || *tmp1 == '|') {
+                length += 2;
+            }
+
+            ++tmp1;
+            ++length;
+        }
+
+        if (VIR_ALLOC_N(annotation, length) < 0) {
+            virReportOOMError();
+            goto cleanup;
+        }
+
+        tmp1 = def->description; /* reading from this one */
+        tmp2 = annotation; /* writing to this one */
+
+        while (*tmp1 != '\0') {
+            if (*tmp1 == '"') {
+                *tmp2++ = '|'; *tmp2++ = '2'; *tmp2++ = '2';
+            } else if (*tmp1 == '|') {
+                *tmp2++ = '|'; *tmp2++ = '7'; *tmp2++ = 'C';
+            } else {
+                *tmp2++ = *tmp1;
+            }
+
+            ++tmp1;
+        }
+
+        *tmp2 = '\0';
+
+        virBufferVSprintf(&buffer, "annotation = \"%s\"\n", annotation);
+    }
+
     /* def:maxmem -> vmx:memsize */
     if (def->maxmem <= 0 || def->maxmem % 4096 != 0) {
         ESX_ERROR(VIR_ERR_INTERNAL_ERROR,
                   _("Expecting domain XML entry 'memory' to be an unsigned "
                     "integer (multiple of 4096) but found %lld"),
                   (unsigned long long)def->maxmem);
-        goto failure;
+        goto cleanup;
     }
 
     /* Scale from kilobytes to megabytes */
@@ -2412,7 +2489,7 @@ esxVMX_FormatConfig(esxVMX_Context *ctx, virCapsPtr caps, virDomainDefPtr def,
                       _("Expecting domain XML entry 'currentMemory' to be an "
                         "unsigned integer (multiple of 1024) but found %lld"),
                       (unsigned long long)def->memory);
-            goto failure;
+            goto cleanup;
         }
 
         /* Scale from kilobytes to megabytes */
@@ -2426,7 +2503,7 @@ esxVMX_FormatConfig(esxVMX_Context *ctx, virCapsPtr caps, virDomainDefPtr def,
                   _("Expecting domain XML entry 'vcpu' to be an unsigned "
                     "integer (1 or a multiple of 2) but found %d"),
                   (int)def->vcpus);
-        goto failure;
+        goto cleanup;
     }
 
     virBufferVSprintf(&buffer, "numvcpus = \"%d\"\n", (int)def->vcpus);
@@ -2448,7 +2525,7 @@ esxVMX_FormatConfig(esxVMX_Context *ctx, virCapsPtr caps, virDomainDefPtr def,
                       _("Expecting domain XML attribute 'cpuset' of entry "
                         "'vcpu' to contains at least %d CPU(s)"),
                       (int)def->vcpus);
-            goto failure;
+            goto cleanup;
         }
 
         for (i = 0; i < def->cpumasklen; ++i) {
@@ -2471,7 +2548,7 @@ esxVMX_FormatConfig(esxVMX_Context *ctx, virCapsPtr caps, virDomainDefPtr def,
         switch (def->graphics[i]->type) {
           case VIR_DOMAIN_GRAPHICS_TYPE_VNC:
             if (esxVMX_FormatVNC(def->graphics[i], &buffer) < 0) {
-                goto failure;
+                goto cleanup;
             }
 
             break;
@@ -2480,7 +2557,7 @@ esxVMX_FormatConfig(esxVMX_Context *ctx, virCapsPtr caps, virDomainDefPtr def,
             ESX_ERROR(VIR_ERR_INTERNAL_ERROR,
                       _("Unsupported graphics type '%s'"),
                       virDomainGraphicsTypeToString(def->graphics[i]->type));
-            goto failure;
+            goto cleanup;
         }
     }
 
@@ -2488,13 +2565,13 @@ esxVMX_FormatConfig(esxVMX_Context *ctx, virCapsPtr caps, virDomainDefPtr def,
     for (i = 0; i < def->ndisks; ++i) {
         if (esxVMX_VerifyDiskAddress(caps, def->disks[i]) < 0 ||
             esxVMX_HandleLegacySCSIDiskDriverName(def, def->disks[i]) < 0) {
-            goto failure;
+            goto cleanup;
         }
     }
 
     if (esxVMX_GatherSCSIControllers(ctx, def, scsi_virtualDev,
                                      scsi_present) < 0) {
-        goto failure;
+        goto cleanup;
     }
 
     for (i = 0; i < 4; ++i) {
@@ -2513,14 +2590,14 @@ esxVMX_FormatConfig(esxVMX_Context *ctx, virCapsPtr caps, virDomainDefPtr def,
         switch (def->disks[i]->device) {
           case VIR_DOMAIN_DISK_DEVICE_DISK:
             if (esxVMX_FormatHardDisk(ctx, def->disks[i], &buffer) < 0) {
-                goto failure;
+                goto cleanup;
             }
 
             break;
 
           case VIR_DOMAIN_DISK_DEVICE_CDROM:
             if (esxVMX_FormatCDROM(ctx, def->disks[i], &buffer) < 0) {
-                goto failure;
+                goto cleanup;
             }
 
             break;
@@ -2528,7 +2605,7 @@ esxVMX_FormatConfig(esxVMX_Context *ctx, virCapsPtr caps, virDomainDefPtr def,
           case VIR_DOMAIN_DISK_DEVICE_FLOPPY:
             if (esxVMX_FormatFloppy(ctx, def->disks[i], &buffer,
                                     floppy_present) < 0) {
-                goto failure;
+                goto cleanup;
             }
 
             break;
@@ -2537,7 +2614,7 @@ esxVMX_FormatConfig(esxVMX_Context *ctx, virCapsPtr caps, virDomainDefPtr def,
             ESX_ERROR(VIR_ERR_INTERNAL_ERROR,
                       _("Unsupported disk device type '%s'"),
                       virDomainDiskDeviceTypeToString(def->disks[i]->device));
-            goto failure;
+            goto cleanup;
         }
     }
 
@@ -2554,7 +2631,7 @@ esxVMX_FormatConfig(esxVMX_Context *ctx, virCapsPtr caps, virDomainDefPtr def,
     /* def:nets */
     for (i = 0; i < def->nnets; ++i) {
         if (esxVMX_FormatEthernet(def->nets[i], i, &buffer) < 0) {
-            goto failure;
+            goto cleanup;
         }
     }
 
@@ -2570,29 +2647,33 @@ esxVMX_FormatConfig(esxVMX_Context *ctx, virCapsPtr caps, virDomainDefPtr def,
     /* def:serials */
     for (i = 0; i < def->nserials; ++i) {
         if (esxVMX_FormatSerial(ctx, def->serials[i], &buffer) < 0) {
-            goto failure;
+            goto cleanup;
         }
     }
 
     /* def:parallels */
     for (i = 0; i < def->nparallels; ++i) {
         if (esxVMX_FormatParallel(ctx, def->parallels[i], &buffer) < 0) {
-            goto failure;
+            goto cleanup;
         }
     }
 
     /* Get final VMX output */
     if (virBufferError(&buffer)) {
         virReportOOMError();
-        goto failure;
+        goto cleanup;
     }
 
-    return virBufferContentAndReset(&buffer);
+    vmx = virBufferContentAndReset(&buffer);
 
-  failure:
-    virBufferFreeAndReset(&buffer);
+  cleanup:
+    if (vmx == NULL) {
+        virBufferFreeAndReset(&buffer);
+    }
+
+    VIR_FREE(annotation);
 
-    return NULL;
+    return vmx;
 }
 
 
diff --git a/tests/vmx2xmldata/vmx2xml-annotation.vmx b/tests/vmx2xmldata/vmx2xml-annotation.vmx
new file mode 100644
index 0000000..405b73c
--- /dev/null
+++ b/tests/vmx2xmldata/vmx2xml-annotation.vmx
@@ -0,0 +1,3 @@
+config.version = "8"
+virtualHW.version = "4"
+annotation = "Some |7Ctext|7C to test the |22escaping|22: |7C|7C|22|22|7C|7C|22|7C |45|73|63|61|70|65|64|21"
diff --git a/tests/vmx2xmldata/vmx2xml-annotation.xml b/tests/vmx2xmldata/vmx2xml-annotation.xml
new file mode 100644
index 0000000..1af45aa
--- /dev/null
+++ b/tests/vmx2xmldata/vmx2xml-annotation.xml
@@ -0,0 +1,16 @@
+<domain type='vmware'>
+  <uuid>00000000-0000-0000-0000-000000000000</uuid>
+  <description>Some |text| to test the &quot;escaping&quot;: ||&quot;&quot;||&quot;| Escaped!</description>
+  <memory>32768</memory>
+  <currentMemory>32768</currentMemory>
+  <vcpu>1</vcpu>
+  <os>
+    <type arch='i686'>hvm</type>
+  </os>
+  <clock offset='utc'/>
+  <on_poweroff>destroy</on_poweroff>
+  <on_reboot>restart</on_reboot>
+  <on_crash>destroy</on_crash>
+  <devices>
+  </devices>
+</domain>
diff --git a/tests/vmx2xmltest.c b/tests/vmx2xmltest.c
index 50e7d0c..67296d6 100644
--- a/tests/vmx2xmltest.c
+++ b/tests/vmx2xmltest.c
@@ -280,6 +280,8 @@ mymain(int argc, char **argv)
     DO_TEST("gsx-in-the-wild-3", "gsx-in-the-wild-3", esxVI_ProductVersion_ESX35);
     DO_TEST("gsx-in-the-wild-4", "gsx-in-the-wild-4", esxVI_ProductVersion_ESX35);
 
+    DO_TEST("annotation", "annotation", esxVI_ProductVersion_ESX35);
+
     virCapabilitiesFree(caps);
 
     return result == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
diff --git a/tests/xml2vmxdata/xml2vmx-annotation.vmx b/tests/xml2vmxdata/xml2vmx-annotation.vmx
new file mode 100644
index 0000000..47d060d
--- /dev/null
+++ b/tests/xml2vmxdata/xml2vmx-annotation.vmx
@@ -0,0 +1,10 @@
+config.version = "8"
+virtualHW.version = "4"
+guestOS = "other"
+uuid.bios = "56 4d 9b ef ac d9 b4 e0-c8 f0 ae a8 b9 10 35 15"
+displayName = "annotation"
+annotation = "Some |7Ctext|7C to test the |22escaping|22: |7C|7C|22|22|7C|7C|22|7C Unescaped!"
+memsize = "4"
+numvcpus = "1"
+floppy0.present = "false"
+floppy1.present = "false"
diff --git a/tests/xml2vmxdata/xml2vmx-annotation.xml b/tests/xml2vmxdata/xml2vmx-annotation.xml
new file mode 100644
index 0000000..402537e
--- /dev/null
+++ b/tests/xml2vmxdata/xml2vmx-annotation.xml
@@ -0,0 +1,9 @@
+<domain type='vmware'>
+  <name>annotation</name>
+  <uuid>564d9bef-acd9-b4e0-c8f0-aea8b9103515</uuid>
+  <description>Some |text| to test the &quot;escaping&quot;: ||&quot;&quot;||&quot;| Unescaped!</description>
+  <memory>4096</memory>
+  <os>
+    <type>hvm</type>
+  </os>
+</domain>
diff --git a/tests/xml2vmxtest.c b/tests/xml2vmxtest.c
index 71ce14a..2a457b4 100644
--- a/tests/xml2vmxtest.c
+++ b/tests/xml2vmxtest.c
@@ -273,6 +273,8 @@ mymain(int argc, char **argv)
     DO_TEST("gsx-in-the-wild-3", "gsx-in-the-wild-3", esxVI_ProductVersion_ESX35);
     DO_TEST("gsx-in-the-wild-4", "gsx-in-the-wild-4", esxVI_ProductVersion_ESX35);
 
+    DO_TEST("annotation", "annotation", esxVI_ProductVersion_ESX35);
+
     virCapabilitiesFree(caps);
 
     return result == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
-- 
1.7.0.4


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