4:36 p.m.
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
*/
This extra output makes our qemu log files _huge_ if doing things like
polling disk/net stats, and prevents us from returning any useful error
message printed from the monitor (say for media/disk eject, disk/net
stats, etc).
I've been running with this patch for a while and haven't hit any issues.
Thanks,
Cole
4:58 a.m.
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
*/
This extra output makes our qemu log files _huge_ if doing things like
polling disk/net stats, and prevents us from returning any useful error
message printed from the monitor (say for media/disk eject, disk/net
stats, etc).
Yes, this is quite horrific output thanks to QEMU's readline emulation
which we can't turn off.
I've been running with this patch for a while and haven't hit
any issues.
if (buf && ((tmp = strstr(buf, "\n(qemu) ")) != NULL)) {
- tmp[0] = '\0';
+ /* 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))) {
+ char *dupptr = strchr(commptr, '\n');
+ if (dupptr) {
+ char *tmpbuf = strdup(dupptr);
+ VIR_FREE(buf);
+ buf = strdup(tmpbuf);
+ }
+ }
This loooks a little overly complex to me, doesn't handle alloction failures
in strdup correctly & leaks tmpbuf. Can't we just simplify it to
if ((commptr = strstr(buf, cmd)))
memmove(buf, commptr, strlen(commptr)+1);
avoiding re-allocation entirely ?
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
10:22 a.m.
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.
Thanks,
Cole
9:13 a.m.
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
9:30 a.m.
Cole Robinson <crobinso(a)redhat.com> wrote:
...
> 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.
diff --git a/src/qemu_driver.c b/src/qemu_driver.c
...
This looks ok to me.
ACK.
1:30 p.m.
On Thu, Dec 11, 2008 at 10:13:05AM -0500, Cole Robinson wrote:
Cole Robinson wrote:
Just want to bump this. I'd like to commit this if there are no
objections.
Opps, I thought you already had comitted it !
ACK from me.
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"));
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
5829
days inactive
5839
days old
5 comments
3 participants
participants (3)
-
Cole Robinson -
Daniel P. Berrange -
Jim Meyering