On Thu, May 20, 2010 at 04:25:54PM -0600, 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? For a directory based storage pool we don't want to include block devices or character devices IMHO. Those are already dealt with via the other types of storage pool. We *do* however want to allow directories, since this allows for enumerating directories used a virtual root FS for container based virt. So this check should be !S_ISREG && S_ISDIR NB, we will of course need to make sure later code handles directories properly - ie don't have a fatal error upon trying to seek() in a directory, or even better skip that bit of code. > > + 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. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :| -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list