On 05/24/2010 06:36 AM, Daniel P. Berrange wrote: > 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. > I've sent an updated patch, but I only copied the existing mode checks from virStorageBackendUpdateVolTargetInfoFD, so these points aren't addressed. They will take a bit more surgery then I expected, since volumes are opened via several different code paths. - Cole -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list