[libvirt] [PATCH v2 2/3] qemu: add helper functions for diskchain checking

[Martin Kletzander]

On 07/26/2013 02:37 PM, Guannan Ren wrote:
> *src/util/virstoragefile.c: Add a helper function to get
> the first name of missing backing files, if the name is NULL,
> it means the diskchain is not broken.
> *src/qemu/qemu_domain.c: qemuDiskChainCheckBroken(disk) to
> check if its chain is broken
> ---
>  src/libvirt_private.syms  |  1 +
>  src/qemu/qemu_domain.c    | 22 ++++++++++++++++++++++
>  src/qemu/qemu_domain.h    |  3 +++
>  src/util/virstoragefile.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
>  src/util/virstoragefile.h |  2 ++
>  5 files changed, 75 insertions(+)
> 
[...]
> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> index cb6df91..e02e28a 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c
> @@ -572,6 +572,13 @@ virFindBackingFile(const char *start, bool start_is_dir, const char *path,
>          goto cleanup;
>      }
>  
> +    if (virFileAccessibleAs(combined, F_OK, getuid(), getgid()) < 0) {
> +        virReportSystemError(errno,
> +                             _("Cannot access backing file '%s'"),
> +                             combined);
> +        goto cleanup;
> +    }
> +

As mentioned before, the pre-existing problem here is that the disk
chain will be reported as broken in case root doesn't have access to the
folder the file is in (for example).  Could we somehow pass a function
or seclabel to fallback to (from the driver)?  Just a hypothetical
question, no need to fix that in this series.

>      if (!(*canonical = canonicalize_file_name(combined))) {
>          virReportSystemError(errno,
>                               _("Can't canonicalize path '%s'"), path);
> @@ -1097,6 +1104,46 @@ virStorageFileGetMetadata(const char *path, int format,
>  }
>  
>  /**
> + * virStorageFileChainCheckBroken
> + *
> + * Check whether CHAIN is broken, return the broken file name if yes,
> + * otherwise return NULL
> + */

This doesn't cope with the functionality, probably missed it when
reworking the function.

> +int
> +virStorageFileChainGetBroken(virStorageFileMetadataPtr chain,
> +                             char **brokenFile)
> +{
> +    virStorageFileMetadataPtr tmp;
> +    char *file = NULL;

no need to have this variable, you can use bokenFile all the way
(probably leftover before some rework).

ACK with those two things fixed.  Both this and the previous (I forgot
to mention it there) should go in after 1.1.1 release.  Even though it
fixes an error, it mainly prepares the field for other patches and I'd
rather wait with this.  Don't hesitate to disagree though, we can
discuss that on IRC with other in case there's huge need for that.

Martin