[libvirt] [PATCH] Drive hot-unplug: reliable parsing of HMP results

[Daniel P. Berrange]

On Mon, Aug 10, 2015 at 04:49:03PM +0100, Stefan Hajnoczi wrote:
> On Mon, Aug 10, 2015 at 4:37 PM, Frank Schreuder <fschreuder at transip.nl> wrote:
> > Hot-unplugging a disk from a guest that supports hot-unplugging generates an error
> > in the libvirt log when running QEMU with the "-msg timestamp=on" flag.
> >
> > 2015-08-06 10:48:59.945+0000: 11662: error : qemuMonitorTextDriveDel:2594 :
> > operation failed: deleting drive-virtio-disk4 drive failed:
> > 2015-08-06T10:48:59.945058Z Device 'drive-virtio-disk4' not found
> >
> > This error is caused because the HMP results are getting prefixed with a timestamp.
> > Parsing the output is not reliable with STRPREFIX as the results can be prefixed with a timestamp.
> >
> > Using strstr ensures that parsing the output works whether the results are prefixed or not.
> >
> > Cc: Stefan Hajnoczi <stefanha at redhat.com>
> > Cc: Daniel P. Berrange <berrange at redhat.com>
> > Signed-off-by: Frank Schreuder <fschreuder at transip.nl>
> > ---
> >
> >  src/qemu/qemu_monitor_text.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c
> > index 2aa0460..d5ef089 100644
> > --- a/src/qemu/qemu_monitor_text.c
> > +++ b/src/qemu/qemu_monitor_text.c
> > @@ -2586,7 +2586,7 @@ int qemuMonitorTextDriveDel(qemuMonitorPtr mon,
> >
> >      /* (qemu) drive_del wark
> >       * Device 'wark' not found */
> > -    } else if (STRPREFIX(reply, "Device '") && (strstr(reply, "not found"))) {
> > +    } else if (strstr(reply, "Device '") && strstr(reply, "not found")) {
> >          /* NB: device not found errors mean the drive was auto-deleted and we
> >           * ignore the error */
> >      } else if (STRNEQ(reply, "")) {
> 
> I'm not very familiar with the libvirt codebase, but perhaps the
> timestamps on error messages should be stripped in the QEMU monitor
> processIO handler functions?
> 
> That way any remaining bugs like this will be resolved too.

That would be right froma strict correctness POV, but looking at the
existing monitor handling code it all uses strstr too, so this is
matching existing practice. The text monitor code is scary enough
that it is probably wise not to try changing it at this stage in
its life.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|