[libvirt] [PATCH 3/3] storage: Check for invalid storage mode before opening
Cole Robinson
crobinso at redhat.com
Fri May 21 17:07:30 UTC 2010
On 05/20/2010 06:25 PM, Eric Blake wrote:
> On 05/20/2010 01:23 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 a single function which runs stat() before any open() call. Directory
>> pools can then proceed along, ignoring the invalid files.
>
> stat() before open() is racy. Better yet is using the
> O_NONBLOCK|O_NOCTTY flags of open(); gnulib guarantees they are defined,
> and I recently lobbied POSIX to guarantee that it will be safe on all
> platforms (it is already safe on Linux):
> http://austingroupbugs.net/view.php?id=141
>
>>
>> +int
>> +virStorageBackendVolOpen(const char *path)
>> +{
>> + int fd;
>> + struct stat sb;
>> +
>> + if (stat(path, &sb) < 0) {
>> + virReportSystemError(errno,
>> + _("cannot stat file '%s'"), path);
>> + return -1;
>> + }
>> +
>> + if (!S_ISREG(sb.st_mode) &&
>> + !S_ISCHR(sb.st_mode) &&
>> + !S_ISBLK(sb.st_mode)) {
>
> Regular files and block devices I can understand, but character devices?
> That includes things like /dev/zero and /dev/null; do those really make
> sense as the backing of a volume store?
>
Not sure honestly, this chunk was copied from other code which already
did the fstat check (just wasn't early enough or so I thought).
>> + VIR_DEBUG("Skipping volume path %s, unexpected file mode.", path);
>> + return -2;
>> + }
>> +
>> + if ((fd = open(path, O_RDONLY)) < 0) {
>> + virReportSystemError(errno,
>> + _("cannot open volume '%s'"),
>> + path);
>
> In other words, I'd rather see open(path,O_RDONLY|O_NONBLOCK|O_NOCTTY)
> then fstat(), and not your stat()/open() sequence.
>
> But the rest of the patch looks sane.
>
Seems to get the job done, updated patch resent.
Thanks,
Cole
More information about the libvir-list
mailing list