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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list