Re: [libvirt] [PATCH v2] storage: Check for invalid storage mode before opening

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.