[libvirt] [PATCH 2/2] Ignore backing file errors in FS storage pool (v3)

[Daniel P. Berrange]

On Thu, Feb 24, 2011 at 02:35:58PM -0700, Eric Blake wrote:
> On 02/24/2011 12:26 AM, Philipp Hahn wrote:
> 
> If you use '[PATCHv3] subject' instead of '[PATCH] subject (v3)', then
> 'git am' won't include the (v3) as part of the commit message (a good
> thing, since once the patch is upstream, no one cares any longer if it
> took three revisions to be ready for upstream).
> 
> > 
> > Currently a single storage volume with a broken backing file will disable the
> > whole storage pool. This can happen when the backing file is on some
> > unavailable network storage or if the backing volume is deleted, while the
> > storage volumes using it are not.
> > Since the storage pool then can not be re-activated, re-creating the missing 
> > or
> > deleting the now useless volumes using libvirt only is impossible.
> > 
> > To "fix" this case, all errors detected during storage pool activation are now
> > (silently) ignored. Errors are still logged by the called functions, which 
> > have
> > more knowledge on the detailed error condition.
> 
> > 
> > diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
> > index ff39d48..0cf4e54 100644
> > --- a/src/storage/storage_backend_fs.c
> > +++ b/src/storage/storage_backend_fs.c
> > @@ -647,11 +647,11 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn ATTRIBUTE_UNUSED,
> >                                                  &vol->allocation,
> >                                                  &vol->capacity,
> >                                                  &vol->target.encryption)) < 0) {
> > -            if (ret == -1)
> > -                goto cleanup;
> > -            else {
> > -                /* Silently ignore non-regular files,
> > -                 * eg '.' '..', 'lost+found', dangling symbolic link */
> > +            if (ret < 0) {
> > +                /* Silently ignore all errors,
> > +                 * eg '.' '..', 'lost+found', dangling symbolic link,
> > +                 * backend file unavailable, file unreadable.
> > +                 * A detailed error report has already been logged. */
> >                  virStorageVolDefFree(vol);
> >                  vol = NULL;
> >                  continue;
> 
> Hmm, is this fix really right?  Or should we be fixing
> virStorageBackendProbeTarget to handle missing backing files in the same
> way that it handles dangling symlinks (by returning -2), while still
> warning the user about real errors (when returning -1)?

By putting the check here, if a backing store can't be probed, we skip
the entire volume. I think what we really want is to keep the volume
itself, but skip just the backing store. This would mean we need to put
the check in the virStorageBackendProbeTarget() method itself, probably
in the bit of code which does

    if (meta.backingStore) {
        *backingStore = meta.backingStore;
        meta.backingStore = NULL;
        if (meta.backingStoreFormat == VIR_STORAGE_FILE_AUTO) {
            if ((*backingStoreFormat
                 = virStorageFileProbeFormat(*backingStore)) < 0) {
                VIR_FORCE_CLOSE(fd);
                goto cleanup;
            }
        } else {

... if that virStorageFileProbeFormat() call returns -1, we can
just log a warning, and set *backingStoreFormat = VIR_STORAGE_FILE_RAW

> I think that it makes sense to create a directory pool no matter how
> botched the contents of the directory are, but would like a second
> opinion from someone more familiar with the storage pool code before
> committing this (danpb is probably most qualified).

Yep, totally agreed - this is why Philip proposed this patch after we
previously discussed the issue.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|