Re: [libvirt] [PATCH] Sanitize qemu monitor output

[Cole Robinson <crobinso redhat com>]

Cole Robinson wrote:
> Daniel P. Berrange wrote:
>> On Mon, Dec 01, 2008 at 05:36:34PM -0500, Cole Robinson wrote:
>>> The attached patch cleans up output we read from the qemu monitor. This
>>> is a simplified form of a patch I posted awhile ago. From the patch:
>>>
>>> /* The monitor doesn't dump clean output after we have written to
>>>  * it. Every character we write dumps a bunch of useless stuff,
>>>  * so the result looks like "cXcoXcomXcommXcommaXcommanXcommand"
>>>  * Try to throw away everything before the first full command
>>>  * occurence, and inbetween the command and the newline starting
>>>  * the response
>>>  */
>>>
> 
> <snip>
> 
>>
>> This loooks a little overly complex to me, doesn't handle alloction failures
>> in strdup correctly & leaks tmpbuf. 
> 
> Argh, yeah, I'm always botching this.
> 
>>      if ((commptr = strstr(buf, cmd)))
>>           memmove(buf, commptr, strlen(commptr)+1);
>>
> 
> Yes that's much simpler, though it doesn't take into account the control
> characters after the command string and before the newline starting the
> command response.
> 
> We could just do:
> 
>      if ((commptr = strstr(buf, cmd)))
>           memmove(buf, commptr, strlen(commptr)+1);
>      if ((dupptr = strchr(buf, '\n'))
>           memmove(buf+strlen(cmd), dupptr, strlen(dupptr)+1);
> 
> I was also trying to avoid having the command string in the 'reply', but
> it was an unnecessary restriction.
> 
> Updated patch attached. It also fixes an error check from the blockstats
> monitor command which wouldn't have worked in the first place, but
> should be accurate now.
> 

Just want to bump this. I'd like to commit this if there are no
objections.

Thanks,
Cole

diff --git a/src/qemu_driver.c b/src/qemu_driver.c
index 0130d61..0c4226c 100644
--- a/src/qemu_driver.c
+++ b/src/qemu_driver.c
@@ -1145,7 +1145,23 @@ qemudMonitorCommand (const struct qemud_driver *driver ATTRIBUTE_UNUSED,
 
         /* Look for QEMU prompt to indicate completion */
         if (buf && ((tmp = strstr(buf, "\n(qemu) ")) != NULL)) {
-            tmp[0] = '\0';
+            char *commptr = NULL, *nlptr = NULL;
+
+            /* Preserve the newline */
+            tmp[1] = '\0';
+
+            /* The monitor doesn't dump clean output after we have written to
+             * it. Every character we write dumps a bunch of useless stuff,
+             * so the result looks like "cXcoXcomXcommXcommaXcommanXcommand"
+             * Try to throw away everything before the first full command
+             * occurence, and inbetween the command and the newline starting
+             * the response
+             */
+            if ((commptr = strstr(buf, cmd)))
+                memmove(buf, commptr, strlen(commptr)+1);
+            if ((nlptr = strchr(buf, '\n')))
+                memmove(buf+strlen(cmd), nlptr, strlen(nlptr)+1);
+
             break;
         }
     pollagain:
@@ -2596,7 +2612,7 @@ static int qemudDomainChangeEjectableMedia(virDomainPtr dom,
 
     if (qemudMonitorCommand(driver, vm, cmd, &reply) < 0) {
         qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
-                         "%s", _("cannot change cdrom media"));
+                         "%s", _("could not change cdrom media"));
         VIR_FREE(cmd);
         return -1;
     }
@@ -2607,7 +2623,7 @@ static int qemudDomainChangeEjectableMedia(virDomainPtr dom,
     DEBUG ("ejectable media change reply: %s", reply);
     if (strstr(reply, "\ndevice ")) {
         qemudReportError (dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
-                          "%s", _("changing cdrom media failed"));
+                          _("changing cdrom media failed: %s"), reply);
         VIR_FREE(reply);
         VIR_FREE(cmd);
         return -1;
@@ -3143,7 +3159,7 @@ qemudDomainBlockStats (virDomainPtr dom,
      * unlikely to be the name of a block device, we can use this
      * to detect if qemu supports the command.
      */
-    if (STRPREFIX (info, "info ")) {
+    if (strstr(info, "\ninfo ")) {
         qemudReportError (dom->conn, dom, NULL, VIR_ERR_NO_SUPPORT,
                           "%s",
                           _("'info blockstats' not supported by this qemu"));