Re: [libvirt] [PATCH v4 1/4] storage: Split out the valid path check
}
+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).
>>>
>> Hmm... Sure I see that... I can make that adjustment. I'll wait a bit
>> before pushing just so see if there's other feedback...
>>
>
> I think that change should be separate from this code motion.
>
OK, so consider patch 1.5/4:
Fix the if (!STRPREFIX(path, "/dev")) to be if (!STRPREFIX(path, "/dev/"))
to ensure a path such as "/device" isn't declared stable.
Signed-off-by: John Ferlan
---
src/storage/storage_backend.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index b07e0d9..e0311e1 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -1680,7 +1680,7 @@ virStorageBackendPoolPathIsStable(const char *path)
if (path == NULL || STREQ(path, "/dev") || STREQ(path, "/dev/"))
return false;
-if (!STRPREFIX(path, "/dev"))
+if (!STRPREFIX(path, "/dev/"))
return false;
return true;
--
2.1.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 1/4] storage: Split out the valid path check
On Mon, Apr 20, 2015 at 12:54:46PM -0400, John Ferlan wrote:
>
>
> On 04/20/2015 12:23 PM, Eric Blake wrote:
> > 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
> >> ---
> >> 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).
> >
> Hmm... Sure I see that... I can make that adjustment. I'll wait a bit
> before pushing just so see if there's other feedback...
>
I think that change should be separate from this code motion.
Jan
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 1/4] storage: Split out the valid path check
On 04/20/2015 12:23 PM, Eric Blake wrote:
> 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
>> ---
>> 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).
>
Hmm... Sure I see that... I can make that adjustment. I'll wait a bit
before pushing just so see if there's other feedback...
John
>
>> -/* 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.
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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
> ---
> 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.
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 1/4] storage: Split out the valid path check
s/valid/stable/ in the subject On Sun, Apr 19, 2015 at 08:38:33PM -0400, 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 > --- > src/storage/storage_backend.c | 26 +- > src/storage/storage_backend.h | 1 + > 2 files changed, 14 insertions(+), 13 deletions(-) > ACK Jan signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v4 1/4] storage: Split out the valid path check
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
---
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;
+
+return true;
+}
/*
* Given a volume path directly in /dev/XXX, iterate over the
@@ -1703,20 +1714,9 @@ virStorageBackendStablePath(virStoragePoolObjPtr pool,
int retry = 0;
int direrr;
-/* 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;
-
/* Logical pools are under /dev but already have stable paths */
-if (pool->def->type == VIR_STORAGE_POOL_LOGICAL)
+if (pool->def->type == VIR_STORAGE_POOL_LOGICAL ||
+!virStorageBackendPoolPathIsStable(pool->def->target.path))
goto ret_strdup;
/* We loop here because /dev/disk/by-{id,path} may not have existed
diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h
index bb1e8d8..85a8a4b 100644
--- a/src/storage/storage_backend.h
+++ b/src/storage/storage_backend.h
@@ -187,6 +187,7 @@ int
virStorageBackendUpdateVolTargetInfoFD(virStorageSourcePtr target,
int fd,
struct stat *sb);
+bool virStorageBackendPoolPathIsStable(const char *path);
char *virStorageBackendStablePath(virStoragePoolObjPtr pool,
const char *devpath,
bool loop);
--
2.1.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
