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