Re: [libvirt] [PATCH v4 1/4] storage: Split out the valid path check

On 04/19/2015 06:38 PM, John Ferlan wrote: > For virStorageBackendStablePath, in order to make decisions in other code > split out the checks regarding whether the pool's target is empty, using /dev, > using /dev/, or doesn't start with /dev > > Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; > --- > src/storage/storage_backend.c | 26 +++++++++++++------------- > src/storage/storage_backend.h | 1 + > 2 files changed, 14 insertions(+), 13 deletions(-) > > diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c > index 0435983..b07e0d9 100644 > --- a/src/storage/storage_backend.c > +++ b/src/storage/storage_backend.c > @@ -1674,6 +1674,17 @@ virStorageBackendUpdateVolTargetInfoFD(virStorageSourcePtr target, > return 0; > } > > +bool > +virStorageBackendPoolPathIsStable(const char *path) > +{ > + if (path == NULL || STREQ(path, "/dev") || STREQ(path, "/dev/")) > + return false; > + > + if (!STRPREFIX(path, "/dev")) > + return false; I think you want "/dev/" here as the prefix to be required; otherwise, "/device" would match the prefix. (This also means that someone using "//dev/..." would fail the check, but that's probably something we don't need to worry about).

> - /* Short circuit if pool has no target, or if its /dev */ > - if (pool->def->target.path == NULL || > - STREQ(pool->def->target.path, "/dev") || > - STREQ(pool->def->target.path, "/dev/")) > - goto ret_strdup; > - > - /* Skip whole thing for a pool which isn't in /dev > - * so we don't mess filesystem/dir based pools > - */ > - if (!STRPREFIX(pool->def->target.path, "/dev")) > - goto ret_strdup; Of course, the bug was pre-existing in the code you are just refactoring from, but now that we've spotted it, we might as well fix it.