On Tue, Apr 07, 2015 at 04:21:00PM -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 <jferlan@xxxxxxxxxx>
> ---
> src/storage/storage_backend.c | 42 +++++++++++++++++++++++++++++-------------
> src/storage/storage_backend.h | 1 +
> 2 files changed, 30 insertions(+), 13 deletions(-)
>
> diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
> index 9322571..77c3be3 100644
> --- a/src/storage/storage_backend.c
> +++ b/src/storage/storage_backend.c
> @@ -1675,6 +1675,25 @@ virStorageBackendUpdateVolTargetInfoFD(virStorageSourcePtr target,
> return 0;
> }
>
> +/*
> + * Check how the storage pool target path is set.
> + *
Answering 'how?' with a bool seems strange to me :)
> + * Returns false if:
> + * 1. No target path set
> + * 2. Target path is /dev or /dev/
> + * 3. Target path does not start with /dev
This comment just restates the function logic, not its intention.
And it's just a matter of time until someone changes the logic without
updating the comment. I think it is not necessary to comment the
function.
> + */
> +bool
> +virStorageBackendPoolUseDevPath(virStoragePoolObjPtr pool)
'UseDevPath' confuses me. How about 'PoolPathIsStable' (or
PoolPathCanBeStable, since we only check if it starts with dev).
Moreover, this takes the pool as an argument, but only looks at the
path. It should either look at the pool type and deal with logical pools
as well, or just take a string as an argument.
> +{
> + if (pool->def->target.path == NULL ||
> + STREQ(pool->def->target.path, "/dev") ||
> + STREQ(pool->def->target.path, "/dev/") ||
> + !STRPREFIX(pool->def->target.path, "/dev"))
> + return false;
This would look better split into separate conditions, like it was in
the original function.
> +
> + return true;
> +}
>
> /*
> * Given a volume path directly in /dev/XXX, iterate over the
> @@ -1704,20 +1723,17 @@ 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 this is a logical pool, then just strdup the passed devpath
> + * and return. Logical pools are under /dev but already have stable
> + * paths, so they don't need to search under the pool target path.
> + *
This comment is too verbose for my taste, I think the original version:
/* Logical pools are under /dev but already have stable paths */
is enough.
> + * For the SCSI/iSCSI pools, if the pool target path isn't under
> + * /dev, then just strdup the passed devpath and return so we don't
> + * mess filesystem/dir based pools
> */
SCSI/iSCSI pools are not filesystem/dir based.
Jan
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list