Re: [PATCH v3] storage: Report error from VolOpen by default

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 04/02/2014 12:36 PM, Eric Blake wrote:
> On 04/02/2014 09:54 AM, Cole Robinson wrote:
>> Currently VolOpen notifies the user of a potentially non-fatal failure by
>> returning -2 and logging a VIR_WARN or VIR_INFO. Unfortunately most
>> callers treat -2 as fatal but don't actually report any message with
>> the error APIs.
>>
>> Rename the VOL_OPEN_ERROR flag to VOL_OPEN_NOERROR. If NOERROR is specified,
>> we preserve the current behavior of returning -2 (there's only one caller
>> that wants this).
>>
>> However in the default case, only return -1, and actually use the error
>> APIs. Fix up a couple callers as a result.
>> ---
>>  src/storage/storage_backend.c      | 77 ++++++++++++++++++++++++--------------
>>  src/storage/storage_backend.h      |  7 ++--
>>  src/storage/storage_backend_fs.c   | 28 +++++---------
>>  src/storage/storage_backend_scsi.c |  3 --
>>  4 files changed, 62 insertions(+), 53 deletions(-)
> 
>> + * Returns -1 on error. If VIR_STORAGE_VOL_OPEN_NOERROR is passed, we
>> + * return -2 if file mode is unexpected or the volume is a dangling
>> + * symbolic link.
> 
> Seems fair; let's see how it works below...
> 
>>   */
>>  int
>>  virStorageBackendVolOpen(const char *path, struct stat *sb,
>> @@ -1288,9 +1289,10 @@ virStorageBackendVolOpen(const char *path, struct stat *sb,
>>  {
>>      int fd, mode = 0;
>>      char *base = last_component(path);
>> +    bool noerror = (flags * VIR_STORAGE_VOL_OPEN_NOERROR);
> 
> Whoops[1] - that returns true for any non-zero flags value.  I think you
> meant s/*/&/
> 

Doh, sorry bout that. Fixed.

>>      } else {
>> -        VIR_WARN("ignoring unexpected type for file '%s'", path);
>>          VIR_FORCE_CLOSE(fd);
>> -        return -2;
>> +        if (noerror) {
>> +            VIR_WARN("ignoring unexpected type for file '%s'", path);
>> +            return -2;
>> +        }
>> +        virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                       _("unexpected type for file '%s'"), path);
>> +        return -1;
> 
> ...so far, so good (all cases of 'return -2' are guarded by noerror, and
> an error is issued before returning -1).
> 
>>      }
>>  
>>      if (virSetBlocking(fd, true) < 0) {
>> +        VIR_FORCE_CLOSE(fd);
>> +        if (noerror) {
>> +            VIR_WARN("unable to set blocking mode for '%s'", path);
>> +            return -2;
>> +        }
> 
> But this one feels wrong[2].  If we get here, we KNOW the fd has the
> expected type, and we successfully opened it.  This is a case where the
> system is hosed, and we should loudly and unconditionally fail with -1,
> rather than returning -2 on noerror.
> 

Agreed, I was just preserving the original behavior. Fixed to unconditionally
raise error and return -1.

> 
>>  /* VolOpenCheckMode flags */
>>  enum {
>> -    VIR_STORAGE_VOL_OPEN_ERROR  = 1 << 0, /* warn if unexpected type
>> -                                           * encountered */
>> +    VIR_STORAGE_VOL_OPEN_NOERROR  = 1 << 0, /* don't error if unexpected type
>> +                                             * encountered, just warn */
>>      VIR_STORAGE_VOL_OPEN_REG    = 1 << 1, /* regular files okay */
>>      VIR_STORAGE_VOL_OPEN_BLOCK  = 1 << 2, /* block files okay */
>>      VIR_STORAGE_VOL_OPEN_CHAR   = 1 << 3, /* char files okay */
>>      VIR_STORAGE_VOL_OPEN_DIR    = 1 << 4, /* directories okay */
> 
> Cosmetic, but alignment of = is now off [3].
> 

Fixed.

>> -            if (virStorageBackendUpdateVolTargetInfo(&vol->backingStore,
>> -                                                     false,
>> -                                        VIR_STORAGE_VOL_OPEN_DEFAULT) < 0) {
>> -                /* The backing file is currently unavailable, the capacity,
>> -                 * allocation, owner, group and mode are unknown. Just log the
>> -                 * error and continue.
>> -                 * Unfortunately virStorageBackendProbeTarget() might already
>> -                 * have logged a similar message for the same problem, but only
>> -                 * if AUTO format detection was used. */
>> -                virReportError(VIR_ERR_INTERNAL_ERROR,
>> -                               _("cannot probe backing volume info: %s"),
>> -                               vol->backingStore.path);
>> -            }
>> +            virStorageBackendUpdateVolTargetInfo(&vol->backingStore, false,
>> +                                                 VIR_STORAGE_VOL_OPEN_DEFAULT);
>> +            /* If this failed, the backing file is currently unavailable,
>> +             * the capacity, allocation, owner, group and mode are unknown.
>> +             * An error message was raised, but we just continue. */
> 
> You may need an ignore_value() here to keep Coverity quiet about an
> unchecked error return [4].
> 
> This touches code that I'm also working on, so I'm interested in getting
> it in sooner to avoid rebase churn over repeated iterations.  ACK if you
> fix [1] and [2] above; and up to you whether changes are needed at [3]
> and [4].
> 

Fixed with ignore_value() and pushed now. Thanks Eric!

- Cole

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]