[libvirt] [PATCH v2] qemu: read backing chain names from qemu

Shanzhi Yu shyu at redhat.com
Fri Mar 13 04:32:21 UTC 2015


I do meet libvirtd crash sometime when test this patch(I also
met it when test v1 yesterday, but can not reproduce it 100%.)

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fffe9d39700 (LWP 25413)]
virJSONValueObjectGetString (object=0x0, key=key at entry=0x7fffe4f72429
"filename") at util/virjson.c:1074
1074        if (object->type != VIR_JSON_TYPE_OBJECT)
(gdb) t a a bt

Thread 6 (Thread 0x7fffe9d39700 (LWP 25413)):
#0  virJSONValueObjectGetString (object=0x0,
key=key at entry=0x7fffe4f72429 "filename") at util/virjson.c:1074
#1  0x00007fffe4f2a1f4 in qemuMonitorJSONDiskNameLookupOne
(image=<optimized out>, top=0x7fffd40013b0,
target=target at entry=0x7fffd40013b0)
    at qemu/qemu_monitor_json.c:3901
#2  0x00007fffe4f2a1bc in qemuMonitorJSONDiskNameLookupOne
(image=<optimized out>, top=top at entry=0x7fffdc0fc940,
target=target at entry=0x7fffd40013b0)
    at qemu/qemu_monitor_json.c:3898
#3  0x00007fffe4f31800 in qemuMonitorJSONDiskNameLookup (mon=<optimized
out>, device=0x7fffd429cee0 "drive-virtio-disk0", top=0x7fffdc0fc940,
    target=target at entry=0x7fffd40013b0) at qemu/qemu_monitor_json.c:3963
#4  0x00007fffe4f1f87e in qemuMonitorDiskNameLookup (mon=<optimized
out>, device=<optimized out>, top=<optimized out>,
target=target at entry=0x7fffd40013b0)
    at qemu/qemu_monitor.c:3475
#5  0x00007fffe4f55775 in qemuDomainBlockCommit (dom=<optimized out>,
path=<optimized out>, base=<optimized out>, top=<optimized out>,
bandwidth=<optimized out>,
    flags=<optimized out>) at qemu/qemu_driver.c:16937
#6  0x00007ffff75ff933 in virDomainBlockCommit
(dom=dom at entry=0x7fffd429d630, disk=0x7fffd40010a0 "vda", base=0x0,
top=0x0, bandwidth=0, flags=5)
    at libvirt-domain.c:10218
#7  0x00005555555736fe in remoteDispatchDomainBlockCommit
(server=<optimized out>, msg=<optimized out>, args=0x7fffd429d9c0,
rerr=0x7fffe9d38cb0,
    client=<optimized out>) at remote_dispatch.h:2594
#8  remoteDispatchDomainBlockCommitHelper (server=<optimized out>,
client=<optimized out>, msg=<optimized out>, rerr=0x7fffe9d38cb0,
args=0x7fffd429d9c0,
    ret=<optimized out>) at remote_dispatch.h:2564
#9  0x00007ffff7653db9 in virNetServerProgramDispatchCall
(msg=0x5555557d8240, client=0x5555557ce4a0, server=0x5555557cc820,
prog=0x5555557d4a40)
    at rpc/virnetserverprogram.c:437
#10 virNetServerProgramDispatch (prog=0x5555557d4a40,
server=server at entry=0x5555557cc820, client=0x5555557ce4a0,
msg=0x5555557d8240) at rpc/virnetserverprogram.c:307
#11 0x00005555555989d8 in virNetServerProcessMsg (msg=<optimized out>,
prog=<optimized out>, client=<optimized out>, srv=0x5555557cc820) at
rpc/virnetserver.c:172
#12 virNetServerHandleJob (jobOpaque=<optimized out>,
opaque=0x5555557cc820) at rpc/virnetserver.c:193
#13 0x00007ffff755ed8e in virThreadPoolWorker
(opaque=opaque at entry=0x5555557d8370) at util/virthreadpool.c:144
#14 0x00007ffff755e72e in virThreadHelper (data=<optimized out>) at
util/virthread.c:197
#15 0x00007ffff5de252a in start_thread (arg=0x7fffe9d39700) at
pthread_create.c:310
#16 0x00007ffff5b1e22d in clone () at
../sysdeps/unix/sysv/linux/x86_64/clone.S:109



On 03/13/2015 04:23 AM, Eric Blake wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1199182 documents that
> after a series of disk snapshots into existing destination images,
> followed by active commits of the top image, it is possible for
> qemu 2.2 and earlier to end up tracking a different name for the
> image than what it would have had when opening the chain afresh.
> That is, when starting with the chain 'a <- b <- c', the name
> associated with 'b' is how it was spelled in the metadata of 'c',
> but when starting with 'a', taking two snapshots into 'a <- b <- c',
> then committing 'c' back into 'b', the name associated with 'b' is
> now the name used when taking the first snapshot.
>
> Sadly, older qemu doesn't know how to treat different spellings of
> the same filename as identical files (it uses strcmp() instead of
> checking for the same inode), which means libvirt's attempt to
> commit an image using solely the names learned from qcow2 metadata
> fails with a cryptic:
>
> error: internal error: unable to execute QEMU command 'block-commit': Top image file /tmp/images/c/../b/b not found
>
> even though the file exists.  Trying to teach libvirt the rules on
> which name qemu will expect is not worth the effort (besides, we'd
> have to remember it across libvirtd restarts, and track whether a
> file was opened via metadata or via snapshot creation for a given
> qemu process); it is easier to just always directly ask qemu what
> string it expects to see in the first place.
>
> As a safety valve, we validate that any name returned by qemu
> still maps to the same local file as we have tracked it, so that
> a compromised qemu cannot accidentally cause us to act on an
> incorrect file.
>
> * src/qemu/qemu_monitor.h (qemuMonitorDiskNameLookup): New
> prototype.
> * src/qemu/qemu_monitor_json.h (qemuMonitorJSONDiskNameLookup):
> Likewise.
> * src/qemu/qemu_monitor.c (qemuMonitorDiskNameLookup): New function.
> * src/qemu/qemu_monitor_json.c (qemuMonitorJSONDiskNameLookup)
> (qemuMonitorJSONDiskNameLookupOne): Likewise.
> * src/qemu/qemu_driver.c (qemuDomainBlockCommit)
> (qemuDomainBlockJobImpl): Use it.
>
> Signed-off-by: Eric Blake <eblake at redhat.com>
> ---
>
> v2: as suggested by Dan, add a sanity checking valve to ensure we
> don't use qemu's string until vetting that it resolves to the same
> local name we are already tracking
>
>  src/qemu/qemu_driver.c       | 28 ++++++-------
>  src/qemu/qemu_monitor.c      | 20 ++++++++-
>  src/qemu/qemu_monitor.h      |  8 +++-
>  src/qemu/qemu_monitor_json.c | 97 +++++++++++++++++++++++++++++++++++++++++++-
>  src/qemu/qemu_monitor_json.h |  9 +++-
>  5 files changed, 144 insertions(+), 18 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index b3263ac..f0e530d 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -16132,9 +16132,6 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm,
>          goto endjob;
>
>      if (baseSource) {
> -        if (qemuGetDriveSourceString(baseSource, NULL, &basePath) < 0)
> -            goto endjob;
> -
>          if (flags & VIR_DOMAIN_BLOCK_REBASE_RELATIVE) {
>              if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_CHANGE_BACKING_FILE)) {
>                  virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> @@ -16172,8 +16169,12 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm,
>      }
>
>      qemuDomainObjEnterMonitor(driver, vm);
> -    ret = qemuMonitorBlockJob(priv->mon, device, basePath, backingPath,
> -                              speed, mode, async);
> +    if (baseSource)
> +        basePath = qemuMonitorDiskNameLookup(priv->mon, device, disk->src,
> +                                             baseSource);
> +    if (!baseSource || basePath)
> +        ret = qemuMonitorBlockJob(priv->mon, device, basePath, backingPath,
> +                                  speed, mode, async);
>      if (qemuDomainObjExitMonitor(driver, vm) < 0)
>          ret = -1;
>      if (ret < 0) {
> @@ -16903,12 +16904,6 @@ qemuDomainBlockCommit(virDomainPtr dom,
>                                             VIR_DISK_CHAIN_READ_WRITE) < 0))
>          goto endjob;
>
> -    if (qemuGetDriveSourceString(topSource, NULL, &topPath) < 0)
> -        goto endjob;
> -
> -    if (qemuGetDriveSourceString(baseSource, NULL, &basePath) < 0)
> -        goto endjob;
> -
>      if (flags & VIR_DOMAIN_BLOCK_COMMIT_RELATIVE &&
>          topSource != disk->src) {
>          if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_CHANGE_BACKING_FILE)) {
> @@ -16939,9 +16934,14 @@ qemuDomainBlockCommit(virDomainPtr dom,
>          disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT;
>      }
>      qemuDomainObjEnterMonitor(driver, vm);
> -    ret = qemuMonitorBlockCommit(priv->mon, device,
> -                                 topPath, basePath, backingPath,
> -                                 speed);
> +    basePath = qemuMonitorDiskNameLookup(priv->mon, device, disk->src,
> +                                         baseSource);
> +    topPath = qemuMonitorDiskNameLookup(priv->mon, device, disk->src,
> +                                        topSource);
> +    if (basePath && topPath)
> +        ret = qemuMonitorBlockCommit(priv->mon, device,
> +                                     topPath, basePath, backingPath,
> +                                     speed);
>      if (qemuDomainObjExitMonitor(driver, vm) < 0) {
>          ret = -1;
>          goto endjob;
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index d869a72..cf7dc5e 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -1,7 +1,7 @@
>  /*
>   * qemu_monitor.c: interaction with QEMU monitor console
>   *
> - * Copyright (C) 2006-2014 Red Hat, Inc.
> + * Copyright (C) 2006-2015 Red Hat, Inc.
>   * Copyright (C) 2006 Daniel P. Berrange
>   *
>   * This library is free software; you can redistribute it and/or
> @@ -3458,6 +3458,24 @@ qemuMonitorSupportsActiveCommit(qemuMonitorPtr mon)
>  }
>
>
> +/* Determine the name that qemu is using for tracking the backing
> + * element TARGET within the chain starting at TOP.  */
> +char *
> +qemuMonitorDiskNameLookup(qemuMonitorPtr mon,
> +                          const char *device,
> +                          virStorageSourcePtr top,
> +                          virStorageSourcePtr target)
> +{
> +    if (!mon->json) {
> +        virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> +                       _("JSON monitor is required"));
> +        return NULL;
> +    }
> +
> +    return qemuMonitorJSONDiskNameLookup(mon, device, top, target);
> +}
> +
> +
>  /* Use the block-job-complete monitor command to pivot a block copy
>   * job.  */
>  int
> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
> index b30da34..e67d800 100644
> --- a/src/qemu/qemu_monitor.h
> +++ b/src/qemu/qemu_monitor.h
> @@ -1,7 +1,7 @@
>  /*
>   * qemu_monitor.h: interaction with QEMU monitor console
>   *
> - * Copyright (C) 2006-2014 Red Hat, Inc.
> + * Copyright (C) 2006-2015 Red Hat, Inc.
>   * Copyright (C) 2006 Daniel P. Berrange
>   *
>   * This library is free software; you can redistribute it and/or
> @@ -734,6 +734,12 @@ int qemuMonitorBlockCommit(qemuMonitorPtr mon,
>      ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
>      ATTRIBUTE_NONNULL(4);
>  bool qemuMonitorSupportsActiveCommit(qemuMonitorPtr mon);
> +char *qemuMonitorDiskNameLookup(qemuMonitorPtr mon,
> +                                const char *device,
> +                                virStorageSourcePtr top,
> +                                virStorageSourcePtr target)
> +    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
> +    ATTRIBUTE_NONNULL(4);
>
>  int qemuMonitorArbitraryCommand(qemuMonitorPtr mon,
>                                  const char *cmd,
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index c16f3ca..522fd17 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -1,7 +1,7 @@
>  /*
>   * qemu_monitor_json.c: interaction with QEMU monitor console
>   *
> - * Copyright (C) 2006-2014 Red Hat, Inc.
> + * Copyright (C) 2006-2015 Red Hat, Inc.
>   * Copyright (C) 2006 Daniel P. Berrange
>   *
>   * This library is free software; you can redistribute it and/or
> @@ -3883,6 +3883,101 @@ qemuMonitorJSONDrivePivot(qemuMonitorPtr mon, const char *device,
>  }
>
>
> +static char *
> +qemuMonitorJSONDiskNameLookupOne(virJSONValuePtr image,
> +                                 virStorageSourcePtr top,
> +                                 virStorageSourcePtr target)
> +{
> +    virJSONValuePtr backing;
> +    char *ret;
> +
> +    if (!top)
> +        return NULL;
> +    if (top != target) {
> +        backing = virJSONValueObjectGet(image, "backing-image");
> +        return qemuMonitorJSONDiskNameLookupOne(backing, top->backingStore,
> +                                                target);
> +    }
> +    if (VIR_STRDUP(ret, virJSONValueObjectGetString(image, "filename")) < 0)
> +        return NULL;
> +    /* Sanity check - the name qemu gave us should resolve to the same
> +       file tracked by our target description. */
> +    if (virStorageSourceIsLocalStorage(target) &&
> +        STRNEQ(ret, target->path) &&
> +        !virFileLinkPointsTo(ret, target->path)) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("qemu block name '%s' doesn't match expected '%s'"),
> +                       ret, target->path);
> +        VIR_FREE(ret);
> +    }
> +    return ret;
> +}
> +
> +
> +char *
> +qemuMonitorJSONDiskNameLookup(qemuMonitorPtr mon,
> +                              const char *device,
> +                              virStorageSourcePtr top,
> +                              virStorageSourcePtr target)
> +{
> +    char *ret = NULL;
> +    virJSONValuePtr cmd = NULL;
> +    virJSONValuePtr reply = NULL;
> +    virJSONValuePtr devices;
> +    size_t i;
> +
> +    cmd = qemuMonitorJSONMakeCommand("query-block", NULL);
> +    if (!cmd)
> +        return NULL;
> +    if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0)
> +        goto cleanup;
> +
> +    devices = virJSONValueObjectGet(reply, "return");
> +    if (!devices || devices->type != VIR_JSON_TYPE_ARRAY) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("block info reply was missing device list"));
> +        goto cleanup;
> +    }
> +
> +    for (i = 0; i < virJSONValueArraySize(devices); i++) {
> +        virJSONValuePtr dev = virJSONValueArrayGet(devices, i);
> +        virJSONValuePtr inserted;
> +        virJSONValuePtr image;
> +        const char *thisdev;
> +
> +        if (!dev || dev->type != VIR_JSON_TYPE_OBJECT) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                           _("block info device entry was not in expected format"));
> +            goto cleanup;
> +        }
> +
> +        if ((thisdev = virJSONValueObjectGetString(dev, "device")) == NULL) {

If dev=NULL, it will cased crash in virJSONValueObjectGetString?

const char *
virJSONValueObjectGetString(virJSONValuePtr object,
                            const char *key)
{
    virJSONValuePtr val;
    if (object->type != VIR_JSON_TYPE_OBJECT)
        return NULL;

> +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                           _("block info device entry was not in expected format"));
> +            goto cleanup;
> +        }
> +
> +        if (STREQ(thisdev, device)) {
> +            if ((inserted = virJSONValueObjectGet(dev, "inserted")) &&
> +                (image = virJSONValueObjectGet(inserted, "image"))) {
> +                ret = qemuMonitorJSONDiskNameLookupOne(image, top, target);
> +            }
> +            break;
> +        }
> +    }
> +    if (!ret && !virGetLastError())
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("unable to find backing name for device %s"),
> +                       device);
> +
> + cleanup:
> +    virJSONValueFree(cmd);
> +    virJSONValueFree(reply);
> +
> +    return ret;
> +}
> +
> +
>  int qemuMonitorJSONArbitraryCommand(qemuMonitorPtr mon,
>                                      const char *cmd_str,
>                                      char **reply_str,
> diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
> index 8ceea8a..49392b6 100644
> --- a/src/qemu/qemu_monitor_json.h
> +++ b/src/qemu/qemu_monitor_json.h
> @@ -1,7 +1,7 @@
>  /*
>   * qemu_monitor_json.h: interaction with QEMU monitor console
>   *
> - * Copyright (C) 2006-2009, 2011-2014 Red Hat, Inc.
> + * Copyright (C) 2006-2009, 2011-2015 Red Hat, Inc.
>   * Copyright (C) 2006 Daniel P. Berrange
>   *
>   * This library is free software; you can redistribute it and/or
> @@ -277,6 +277,13 @@ int qemuMonitorJSONBlockCommit(qemuMonitorPtr mon,
>                                 unsigned long long bandwidth)
>      ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
>
> +char *qemuMonitorJSONDiskNameLookup(qemuMonitorPtr mon,
> +                                    const char *device,
> +                                    virStorageSourcePtr top,
> +                                    virStorageSourcePtr target)
> +    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
> +    ATTRIBUTE_NONNULL(4);
> +
>  int qemuMonitorJSONArbitraryCommand(qemuMonitorPtr mon,
>                                      const char *cmd_str,
>                                      char **reply_str,

-- 
Regards
shyu




More information about the libvir-list mailing list