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