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

[Stefan Hajnoczi]

On Mon, Aug 10, 2015 at 05:01:22PM +0100, Daniel P. Berrange wrote:
> 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.

Fair enough.

Stefan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150811/92fc155b/attachment-0001.sig>