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

Guannan Ren gren at redhat.com
Mon Jul 29 12:48:59 UTC 2013


On 07/29/2013 07:27 PM, Martin Kletzander wrote:
> 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.

yeah, right, I will try to resolve it in separate patch.


>
>>       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

Thanks for the review. I will send a v3 version with these fixed




More information about the libvir-list mailing list