On Fri, May 21, 2010 at 02:24:24PM -0400, Cole Robinson wrote: > On 05/21/2010 02:17 PM, Eric Blake wrote: > > On 05/21/2010 11:03 AM, 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 > > > > This sentence of the commit log is out-of-date... > > > >> pools can then proceed along, ignoring the invalid files. > >> > >> v2: Use ATTRIBUTE_NONNULL. Drop stat check, just open with > >> O_NONBLOCK|O_NOCTTY. > > > > ...given this sentence. > > > >> +int > >> +virStorageBackendVolOpen(const char *path) > >> +{ > >> + int fd; > >> + > >> + if ((fd = open(path, O_RDONLY|O_NONBLOCK|O_NOCTTY)) < 0) { > >> + virReportSystemError(errno, > >> + _("cannot open volume '%s'"), > >> + path); > >> + return -1; > >> + } > > > > I'm thinking that we still want to use fstat() (after the open()) and > > reject directories, sockets, and pipes (or more accurately, accept only > > regular files, block devices, and possibly character devices). > > > > In other words, you got rid of the stat/open race (good), but also got > > rid of the strictness provided by validating [f]stat (not so good). > > > > Sorry, I should have pointed out that there is already code that does > this, see virStorageBackendUpdateVolTargetInfoFD, which is always called > after VolOpen. The existing code is all confusingly arranged though, so > maybe I should take a harder look at organizing. That check isn't sufficient. We need to explicitly *allow* directories to be reported, since a pool may point at a directory containing trees for container based virt root filesystems. We should drop block+char devices for directory based pools too, since those aren't relevant, being dealt with by other storage pool backends. 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