Re: [PATCH v2] multipath: remove duplicates from multipath configuration

[Hannes Reinecke <hare@suse.de>]

On 01/10/2013 09:10 PM, Benjamin Marzinski wrote:
> Added code to remove duplcate entries in the devices section, and the
> blacklist devices section of the builtin configuration table. The only
> change to setup_default_blist is the addition of _blacklist_device()
> to check if the device's bl_product entry already exists.
>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>   libmultipath/blacklist.c |   91 ++++++++++++++++++++++++-----------------------
>   libmultipath/config.c    |   16 ++++++--
>   2 files changed, 60 insertions(+), 47 deletions(-)
>
> Index: multipath-tools-120821/libmultipath/blacklist.c
> ===================================================================
> --- multipath-tools-120821.orig/libmultipath/blacklist.c
> +++ multipath-tools-120821/libmultipath/blacklist.c
> @@ -96,50 +96,6 @@ set_ble_device (vector blist, char * ven
>   }
>
>   int
> -setup_default_blist (struct config * conf)
> -{
> -	struct blentry * ble;
> -	struct hwentry *hwe;
> -	char * str;
> -	int i;
> -
> -	str = STRDUP("^(ram|raw|loop|fd|md|dm-|sr|scd|st)[0-9]*");
> -	if (!str)
> -		return 1;
> -	if (store_ble(conf->blist_devnode, str, ORIGIN_DEFAULT))
> -		return 1;
> -
> -	str = STRDUP("^hd[a-z]");
> -	if (!str)
> -		return 1;
> -	if (store_ble(conf->blist_devnode, str, ORIGIN_DEFAULT))
> -		return 1;
> -
> -	str = STRDUP("^dcssblk[0-9]*");
> -	if (!str)
> -		return 1;
> -	if (store_ble(conf->blist_devnode, str, ORIGIN_DEFAULT))
> -		return 1;
> -
> -	vector_foreach_slot (conf->hwtable, hwe, i) {
> -		if (hwe->bl_product) {
> -			if (alloc_ble_device(conf->blist_device))
> -				return 1;
> -			ble = VECTOR_SLOT(conf->blist_device,
> -					  VECTOR_SIZE(conf->blist_device) -1);
> -			if (set_ble_device(conf->blist_device,
> -					   STRDUP(hwe->vendor),
> -					   STRDUP(hwe->bl_product),
> -					   ORIGIN_DEFAULT)) {
> -				FREE(ble);
> -				return 1;
> -			}
> -		}
> -	}
> -	return 0;
> -}
> -
> -int
>   _blacklist_exceptions (vector elist, char * str)
>   {
>   	int i;
> @@ -192,6 +148,53 @@ _blacklist_device (vector blist, char *
>   	}
>   	return 0;
>   }
> +
> +int
> +setup_default_blist (struct config * conf)
> +{
> +	struct blentry * ble;
> +	struct hwentry *hwe;
> +	char * str;
> +	int i;
> +
> +	str = STRDUP("^(ram|raw|loop|fd|md|dm-|sr|scd|st)[0-9]*");
> +	if (!str)
> +		return 1;
> +	if (store_ble(conf->blist_devnode, str, ORIGIN_DEFAULT))
> +		return 1;
> +
> +	str = STRDUP("^hd[a-z]");
> +	if (!str)
> +		return 1;
> +	if (store_ble(conf->blist_devnode, str, ORIGIN_DEFAULT))
> +		return 1;
> +
> +	str = STRDUP("^dcssblk[0-9]*");
> +	if (!str)
> +		return 1;
> +	if (store_ble(conf->blist_devnode, str, ORIGIN_DEFAULT))
> +		return 1;
> +
> +	vector_foreach_slot (conf->hwtable, hwe, i) {
> +		if (hwe->bl_product) {
> +			if (_blacklist_device(conf->blist_device, hwe->vendor,
> +					      hwe->bl_product))
> +				continue;
> +			if (alloc_ble_device(conf->blist_device))
> +				return 1;
> +			ble = VECTOR_SLOT(conf->blist_device,
> +					  VECTOR_SIZE(conf->blist_device) -1);
> +			if (set_ble_device(conf->blist_device,
> +					   STRDUP(hwe->vendor),
> +					   STRDUP(hwe->bl_product),
> +					   ORIGIN_DEFAULT)) {
> +				FREE(ble);
> +				return 1;
> +			}
> +		}
> +	}
> +	return 0;
> +}
>
>   #define LOG_BLIST(M) \
>   	if (vendor && product)						 \
> Index: multipath-tools-120821/libmultipath/config.c
> ===================================================================
> --- multipath-tools-120821.orig/libmultipath/config.c
> +++ multipath-tools-120821/libmultipath/config.c
> @@ -25,13 +25,16 @@
>   static int
>   hwe_strmatch (struct hwentry *hwe1, struct hwentry *hwe2)
>   {
> -	if (hwe1->vendor && hwe2->vendor && strcmp(hwe1->vendor, hwe2->vendor))
> +	if ((!!(hwe1->vendor) != !!(hwe2->vendor)) ||
> +	    (hwe1->vendor && strcmp(hwe1->vendor, hwe2->vendor)))
>   		return 1;
>
> -	if (hwe1->product && hwe2->product && strcmp(hwe1->product, hwe2->product))
> +	if ((!!(hwe1->product) != !!(hwe2->product)) ||
> +	    (hwe1->product && strcmp(hwe1->product, hwe2->product)))
>   		return 1;
>
> -	if (hwe1->revision && hwe2->revision && strcmp(hwe1->revision, hwe2->revision))
> +	if ((!!(hwe1->revision) != !!(hwe2->revision)) ||
> +	    (hwe1->revision && strcmp(hwe1->revision, hwe2->revision)))
>   		return 1;
>
>   	return 0;
I hate the '!!' constructs.

> @@ -416,6 +419,13 @@ factorize_hwtable (vector hw, int n)
>   				continue;
>   			/* dup */
>   			merge_hwe(hwe2, hwe1);
> +			if (hwe_strmatch(hwe2, hwe1) == 0) {
> +				vector_del_slot(hw, i);
> +				free_hwe(hwe1);
> +				n -= 1;
> +				i -= 1;
> +				j -= 1;
> +			}
>   		}
>   	}
>   	return 0;
>
Is the 'hwe_strmatch' required here?
Also I'm not sure if just decrementing the indices is the correct
way of doing things; I had a version which just restarted
the outer loop if we encountered a duplicate.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)