On 05/28/2010 12:55 PM, Eric Blake wrote: > On 05/24/2010 03:36 PM, 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 helper functions, which use the proper open() flags to avoid error, >> followed by fstat to validate storage mode. >> >> --- a/src/storage/storage_backend.c >> +++ b/src/storage/storage_backend.c >> @@ -872,6 +872,61 @@ virStorageBackendForType(int type) { >> return NULL; >> } >> >> +static int >> +virStorageBackendVolOpenInternal(const char *path) >> +{ >> + int fd; >> + struct stat sb; >> + >> + if ((fd = open(path, O_RDONLY|O_NONBLOCK|O_NOCTTY)) < 0) { >> + virReportSystemError(errno, >> + _("cannot open volume '%s'"), >> + path); >> + return -1; >> + } >> + >> + if (fstat(fd, &sb) < 0) { >> + virReportSystemError(errno, >> + _("cannot stat file '%s'"), >> + path); >> + close(fd); >> + return -1; >> + } >> + >> + if (!S_ISREG(sb.st_mode) && >> + !S_ISCHR(sb.st_mode) && >> + !S_ISBLK(sb.st_mode)) { >> + close(fd); >> + return -2; >> + } >> + >> + return fd; >> +} >> + >> +/* >> + * Allows caller to silently ignore files with improper mode >> + * >> + * Returns -1 on error, -2 if file mode is unexpected. >> + */ >> +int virStorageBackendVolOpenModeSkip(const char *path) >> +{ >> + return virStorageBackendVolOpenInternal(path); >> +} >> + >> +int >> +virStorageBackendVolOpen(const char *path) > > Hmm, I'm thinking that better semantics might be: > > enum { > VIR_STORAGE_OPEN_WARN = 1 << 0; // warn if unexpected type encountered > VIR_STORAGE_OPEN_REG = 1 << 1; // regular files okay > VIR_STORAGE_OPEN_BLOCK = 1 << 2; // block files okay > VIR_STORAGE_OPEN_DIR = 1 << 3; // directories okay > ... > } > > int virStorageBackendVolOpen(const char *path) { > return virStorageBackendVolOpenCheckMode(path, 0); > } > > int virStorageBackendVolOpenCheckMode(const char *path, > unsigned int flags) { > open > fstat > if (flags & VIR_STORAGE_OPEN_WARN) { > if (type == REG && !(flags & VIR_STORAGE_OPEN_REG)) > warn > else if (type == DIR && !(flags & VIR_STORAGE_OPEN_DIR)) > warn > ... > } > return result; > } > > Then, you can preserve existing semantics of no warning by calling the > one-argument version, or fine-tune which file types you are willing to > accept (some callers will accept directories, others will not) by > calling the two-arg version. > Thanks, I've sent a new version. It doesn't add the directory handling or change the FS backend values, I think that's best saved for another patch so we aren't mixing bug fixing with enhancement. Thanks, Cole -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list