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.

+            } else if (STRPREFIX(tmp, "|7C") || STRPREFIX(tmp, "|7c")) {
+                *tmp = '|';

Dead assignment since we already know *tmp is '|'. Then again, if you change to accept all escapes, this line would disappear anyways.

@@ -2314,10 +2345,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;
+    int length;

s/int/size_t/

The rest of the patch looks okay to me, however.

--
Eric Blake   ebl...@redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Reply via email to