[libvirt] [PATCH 2/4] qemu: report error if disk backing files doesn't exist

[Guannan Ren]

On 07/25/2013 10:17 PM, Martin Kletzander wrote:
> On 07/18/2013 01:32 PM, Guannan Ren wrote:
>
> s/doesn't/don't/ in $SUBJ
>
>> Adding virFileAccessibleAs() to check if the backing file described in
>> disk meta exist in real path. If not, report error. The uid and gid
>> arguments don't take effect on F_OK mode for access, so use gid and gid
>> of current process.
>>
> This patch doesn't break anything new, but thanks to the
> getuid()/getgid(), it leaves the previous problem in the code.  Even
> though F_OK doesn't need uid/gid to check whether the file exists, root
> may not have permissions for upper directories on NFS storage for
> example, so ...
>
>> ---
>>   src/util/virstoragefile.c | 23 +++++++++++++++--------
>>   tests/virstoragetest.c    | 16 ++++++++--------
>>   2 files changed, 23 insertions(+), 16 deletions(-)
>>
>> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
>> index cb6df91..b678eb8 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,
>> +                             _("Backing file '%s' does not exist"),
>> +                             combined);
>> +        goto cleanup;
>> +    }
>> +
>>       if (!(*canonical = canonicalize_file_name(combined))) {
>>           virReportSystemError(errno,
>>                                _("Can't canonicalize path '%s'"), path);
> ... this hunk does make the code report better errors, but in the future
> it should canonicalize the filename using root/qemu/domain users.
>
> ACK to this hunk, with the error reworded (e.g. "Cannot access backing
> file %s") and, of course, commit message changed appropriately, but ...
>
>> @@ -857,14 +864,10 @@ virStorageFileGetMetadataInternal(const char *path,
>>                                          !!directory, backing,
>>                                          &meta->directory,
>>                                          &meta->backingStore) < 0) {
>> -                    /* the backing file is (currently) unavailable, treat this
>> -                     * file as standalone:
>> -                     * backingStoreRaw is kept to mark broken image chains */
>> -                    meta->backingStoreIsFile = false;
>> -                    backingFormat = VIR_STORAGE_FILE_NONE;
>> -                    VIR_WARN("Backing file '%s' of image '%s' is missing.",
>> -                             meta->backingStoreRaw, path);
>> -
>> +                    VIR_FREE(backing);
>> +                    VIR_ERROR(_("Backing file '%s' of image '%s' is missing."),
>> +                              meta->backingStoreRaw, path);
>> +                    goto cleanup;
> To fix a pre-existing error, we should (instead of this change) just add
> virResetLastError() here as the error is used somewhere else in the code
> and should be kept in virFindBackingFile().  Having it in the logs seems
> OK to me.
>

It makes sense, but it seems that the tests/virstoragetest.c testcase is 
using the
last error in checking warning flag, see:
     if (data->flags & EXP_WARN) {
         if (!virGetLastError()) {
             fprintf(stderr, "call should have warned\n");
             goto cleanup;
         }
         virResetLastError();

It is not serious issue without calling virResetLastError() here.