Re: [libvirt] [PATCH v4 2/4] storage: report error rather than warning if backing files doesn't exist

On Wed, Jul 17, 2013 at 10:08:55AM +0800, Guannan Ren wrote: > On 07/16/2013 04:40 PM, Daniel P. Berrange wrote: >> On Tue, Jul 16, 2013 at 08:14:54AM +0200, Martin Kletzander wrote: >>> On 07/15/2013 05:31 PM, Ján Tomko wrote: >>>> On 07/02/2013 11:35 AM, Guannan Ren wrote: >>>>> If one of backing files for disk source doesn't exist, the guest will not >>>>> be able to find and use the disk even though the disk still exists in >>>>> guest xml definition. So reporting an error make more sense. >>>>> >>>>> 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 have so much meannings for F_OK, so give 0 for them. >>>>> --- >>>>> 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 27aa4fe..cb61e5b 100644 >>>>> --- a/src/util/virstoragefile.c >>>>> +++ b/src/util/virstoragefile.c >>>>> @@ -870,14 +877,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_ERROR(_("Backing file '%s' of image '%s' is missing."), >>>>> + meta->backingStoreRaw, path); >>>>> + VIR_FREE(backing); >>>>> + goto cleanup; >>>>> } >>>>> } >>>>> VIR_FREE(backing); >>>> This change means you won't be able to start the pool if one of the files is >>>> missing a backing file. I've forwarded a patch [1] from/for [2] that ignores >>>> missing files on pool start and there is a bug [3] requesting that we ignore >>>> other files as well. I feel like this is going in the other direction. >>>> >>>> Wouldn't it be enough to check for them on domain start-up and leave the pool >>>> running even if one of those volumes doesn't have an existing backing file? >>>> >>> How about making it configurable for the pool? There are definitely >>> some users who want the pool to reflect actual info after pool-refresh. >> I don't think this needs to be configurable. The pool should show *every* >> single file, regardless of whether the file has a broken backing file. >> We shouldn't be trying to second guess what the user wants to do with a >> image with broken backing file. Just expose as much info as we have and >> let them deal with the problem. >> > Daniel, > > How about for guest cold bootup in this situation where one of its > disks has a broken file > we should throw an error out or give a warn in the log as what it is > doing currently. If we can't access the file on guest boot, then we need to raise errors, since it wouldn't be possible to set labelling or permissions. Of course we have to make sure we don't give bogus errors in the case of root squash NFS too. We need to consider storage pool enumeration separately from guest bootup though. Both have specific semantics and we shouldn't force them to behave the same way - the API needs to serve both their needs.