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

Re: [libvirt] [PATCH v3] storage: Check for invalid storage mode before opening



On 05/28/2010 12:55 PM, Eric Blake wrote:
> On 05/24/2010 03:36 PM, Cole Robinson wrote:
>> If a directory pool contains pipes or sockets, a pool start can fail or hang:
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=589577
>>
>> We already try to avoid these special files, but only attempt after
>> opening the path, which is where the problems lie. Unify volume opening
>> into helper functions, which use the proper open() flags to avoid error,
>> followed by fstat to validate storage mode.
>>
>> --- a/src/storage/storage_backend.c
>> +++ b/src/storage/storage_backend.c
>> @@ -872,6 +872,61 @@ virStorageBackendForType(int type) {
>>      return NULL;
>>  }
>>  
>> +static int
>> +virStorageBackendVolOpenInternal(const char *path)
>> +{
>> +    int fd;
>> +    struct stat sb;
>> +
>> +    if ((fd = open(path, O_RDONLY|O_NONBLOCK|O_NOCTTY)) < 0) {
>> +        virReportSystemError(errno,
>> +                             _("cannot open volume '%s'"),
>> +                             path);
>> +        return -1;
>> +    }
>> +
>> +    if (fstat(fd, &sb) < 0) {
>> +        virReportSystemError(errno,
>> +                             _("cannot stat file '%s'"),
>> +                             path);
>> +        close(fd);
>> +        return -1;
>> +    }
>> +
>> +    if (!S_ISREG(sb.st_mode) &&
>> +        !S_ISCHR(sb.st_mode) &&
>> +        !S_ISBLK(sb.st_mode)) {
>> +        close(fd);
>> +        return -2;
>> +    }
>> +
>> +    return fd;
>> +}
>> +
>> +/*
>> + * Allows caller to silently ignore files with improper mode
>> + *
>> + * Returns -1 on error, -2 if file mode is unexpected.
>> + */
>> +int virStorageBackendVolOpenModeSkip(const char *path)
>> +{
>> +    return virStorageBackendVolOpenInternal(path);
>> +}
>> +
>> +int
>> +virStorageBackendVolOpen(const char *path)
> 
> Hmm, I'm thinking that better semantics might be:
> 
> enum {
>   VIR_STORAGE_OPEN_WARN = 1 << 0; // warn if unexpected type encountered
>   VIR_STORAGE_OPEN_REG = 1 << 1; // regular files okay
>   VIR_STORAGE_OPEN_BLOCK = 1 << 2; // block files okay
>   VIR_STORAGE_OPEN_DIR = 1 << 3; // directories okay
>   ...
> }
> 
> int virStorageBackendVolOpen(const char *path) {
>     return virStorageBackendVolOpenCheckMode(path, 0);
> }
> 
> int virStorageBackendVolOpenCheckMode(const char *path,
>       unsigned int flags) {
>   open
>   fstat
>   if (flags & VIR_STORAGE_OPEN_WARN) {
>     if (type == REG && !(flags & VIR_STORAGE_OPEN_REG))
>        warn
>     else if (type == DIR && !(flags & VIR_STORAGE_OPEN_DIR))
>        warn
>     ...
>   }
>   return result;
> }
> 
> Then, you can preserve existing semantics of no warning by calling the
> one-argument version, or fine-tune which file types you are willing to
> accept (some callers will accept directories, others will not) by
> calling the two-arg version.
> 

Thanks, I've sent a new version. It doesn't add the directory handling
or change the FS backend values, I think that's best saved for another
patch so we aren't mixing bug fixing with enhancement.

Thanks,
Cole


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