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

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.