Re: [PATCHv3 17/19] blockjob: implement shallow commit flag in qemu

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 10/17/2012 06:30 PM, Eric Blake wrote:
> Now that we can crawl the chain of backing files, we can do
> argument validation and implement the 'shallow' flag.  In
> testing this, I discovered that it can be handy to pass the
> shallow flag and an explicit base, as a means of validating
> that the base is indeed the file we expected.
>
> * src/qemu/qemu_driver.c (qemuDomainBlockCommit): Crawl through
> chain to implement shallow flag.
> * src/libvirt.c (virDomainBlockCommit): Relax API.
> ---
>
> v3: rebase to changes earlier in series
>
>  src/libvirt.c          |  2 --
>  src/qemu/qemu_driver.c | 63 ++++++++++++++++++++++++++++++++++++++++----------
>  2 files changed, 51 insertions(+), 14 deletions(-)
>
> diff --git a/src/libvirt.c b/src/libvirt.c
> index e3ddf27..6d65965 100644
> --- a/src/libvirt.c
> +++ b/src/libvirt.c
> @@ -19381,8 +19381,6 @@ int virDomainBlockCommit(virDomainPtr dom, const char *disk,
>      }
>
>      virCheckNonNullArgGoto(disk, error);
> -    if (flags & VIR_DOMAIN_BLOCK_COMMIT_SHALLOW)
> -        virCheckNullArgGoto(base, error);
>
>      if (conn->driver->domainBlockCommit) {
>          int ret;
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index c63d16b..73804fe 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -12664,8 +12664,12 @@ qemuDomainBlockCommit(virDomainPtr dom, const char *path, const char *base,
>      int ret = -1;
>      int idx;
>      virDomainDiskDefPtr disk;
> +    const char *top_canon = NULL;
> +    virStorageFileMetadataPtr top_meta = NULL;
> +    const char *top_parent = NULL;
> +    const char *base_canon = NULL;
>
> -    virCheckFlags(0, -1);
> +    virCheckFlags(VIR_DOMAIN_BLOCK_COMMIT_SHALLOW, -1);
>
>      if (!(vm = qemuDomObjFromDomain(dom)))
>          goto cleanup;
> @@ -12696,20 +12700,55 @@ qemuDomainBlockCommit(virDomainPtr dom, const char *path, const char *base,
>                         disk->dst);
>          goto endjob;
>      }
> +    if (qemuDomainDetermineDiskChain(driver, disk, false) < 0)
> +        goto endjob;
>
> -    if (!top)
> -        top = disk->src;
> +    if (!top) {
> +        top_canon = disk->src;
> +        top_meta = disk->backingChain;
> +    } else if (!(top_canon = virStorageFileChainLookup(disk->backingChain,
> +                                                       disk->src,
> +                                                       top, &top_meta,
> +                                                       &top_parent))) {
> +        virReportError(VIR_ERR_INVALID_ARG,
> +                       _("could not find top '%s' in chain for '%s'"),
> +                       top, path);
> +        goto endjob;
> +    }
> +    if (!top_meta || !top_meta->backingStore) {
> +        virReportError(VIR_ERR_INVALID_ARG,
> +                       _("top '%s' in chain for '%s' has no backing file"),
> +                       top, path);
> +        goto endjob;
> +    }
> +    if (!base && (flags & VIR_DOMAIN_BLOCK_COMMIT_SHALLOW)) {
> +        base_canon = top_meta->backingStore;
> +    } else if (!(base_canon = virStorageFileChainLookup(top_meta, top_canon,
> +                                                        base, NULL, NULL))) {
> +        virReportError(VIR_ERR_INVALID_ARG,
> +                       _("could not find base '%s' below '%s' in chain "
> +                         "for '%s'"),
> +                       base ? base : "(default)", top_canon, path);
> +        goto endjob;
> +    }
> +    /* Note that this code exploits the fact that
> +     * virStorageFileChainLookup guarantees a simple pointer
> +     * comparison will work, rather than needing full-blown STREQ.  */
> +    if ((flags & VIR_DOMAIN_BLOCK_COMMIT_SHALLOW) &&
> +        base_canon != top_meta->backingStore) {
> +        virReportError(VIR_ERR_INVALID_ARG,
> +                       _("base '%s' is not immediately below '%s' in chain "
> +                         "for '%s'"),
> +                       base, top_canon, path);
> +        goto endjob;
> +    }
>
> -    /* XXX For now, we are relying on qemu to check that 'top' and
> -     * 'base' resolve to members of the backing chain in correct
> -     * order; but if we ever get more paranoid and track the backing
> -     * chain ourself, we should be pre-validating the data rather than
> -     * relying on qemu.  For that matter, we need to be changing the
> -     * SELinux label on both 'base' and the parent of 'top', so that
> -     * qemu can open(O_RDWR) those files for the duration of the
> -     * commit.  */
> +    /* XXX We need to be changing the SELinux label on both 'base' and
> +     * the parent of 'top', so that qemu can open(O_RDWR) those files
> +     * for the duration of the commit.  */
>      qemuDomainObjEnterMonitor(driver, vm);
> -    ret = qemuMonitorBlockCommit(priv->mon, device, top, base, bandwidth);
> +    ret = qemuMonitorBlockCommit(priv->mon, device, top_canon, base_canon,
> +                                 bandwidth);
>      qemuDomainObjExitMonitor(driver, vm);
>
>  endjob:

Previous ACK stands.

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list


[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]