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

Re: [libvirt] [PATCH] Eliminate large stack buffer in doTunnelSendAll

On 03/05/2010 04:40 PM, Eric Blake wrote:
On 03/05/2010 02:30 PM, Laine Stump wrote:
doTunnelSendAll function (used by QEMU migration) uses a 64k buffer on
the stack, which could be problematic. This patch replaces that with a
buffer from the heap.
There's a lot more large stacks noted by Coverity, but this is a good start.

Heh. Yeah, this one happened to catch my eye last week while I was in this file, and I couldn't let it stand ;-)

While in the neighborhood, this patch also improves error reporting in
the case that saferead fails - previously, virStreamAbort() was called
(resetting errno) before reporting the error. It's been changed to
report the error first.

-    char buffer[65536];
-    int nbytes = sizeof(buffer);
+    char *buffer;
+    int nbytes = TUNNEL_SEND_BUF_SIZE;
+    if (VIR_ALLOC_N(buffer, TUNNEL_SEND_BUF_SIZE)<  0) {
+        virStreamAbort(st);
+        virReportOOMError();
Is this backwards?

Order isn't as important in this case as the other (the one I changed), since virReportOOMError() doesn't need to look at errno. But I agree it would be more consistent to do them both in the same order. I just resubmitted (and forgot to set the In-Reply-To header so it would show up in this thread. #&%$!)

+        return -1;
+    }

      /* XXX should honour the 'resource' parameter here */
      for (;;) {
          nbytes = saferead(sock, buffer, nbytes);
          if (nbytes<  0) {
-            virStreamAbort(st);
              virReportSystemError(errno, "%s",
                                   _("tunnelled migration failed to read from qemu"));
+            virStreamAbort(st);
+            VIR_FREE(buffer);
Especially given that you just reordered the virStreamAbort to come last
here?  If the above was not a mistake, then ACK to the patch.

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