Re: [libvirt] [PATCH 1/9] storage: Refactor iSCSI Source matching
On 04/13/2015 05:27 AM, Peter Krempa wrote:
On Thu, Apr 02, 2015 at 13:39:38 -0400, John Ferlan wrote:
> Create a separate iSCSI Source matching subroutine. Makes the calling
> code a bit cleaner as well as sets up for future patches which need to
> do better source hosts[0].name processing/checking
>
> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
> ---
> src/conf/storage_conf.c | 35 ++++++++++++++++++++++++-----------
> 1 file changed, 24 insertions(+), 11 deletions(-)
>
> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
> index 8b1898b..4a38416 100644
> --- a/src/conf/storage_conf.c
> +++ b/src/conf/storage_conf.c
> @@ -2291,6 +2291,28 @@ matchSCSIAdapterParent(virStoragePoolObjPtr pool,
> }
>
>
> +static bool
> +matchISCSISource(virStoragePoolObjPtr matchpool,
Please use the virStorageConf... prefix for the new function.
How about "virStoragePoolSourceISCSIMatch"
> + virStoragePoolDefPtr def)
> +{
> + if (matchpool->def->source.nhost == 1 && def->source.nhost ==
1) {
> + if (STREQ(matchpool->def->source.hosts[0].name,
> + def->source.hosts[0].name)) {
> + if ((matchpool->def->source.initiator.iqn) &&
> + (def->source.initiator.iqn)) {
> + if (STREQ(matchpool->def->source.initiator.iqn,
> + def->source.initiator.iqn))
> + return true;
> + else
> + return false;
Um, how about return STREQ(... ?
myopia, but in the long run it won't matter as I agree with your view to
merge patches 1-3 (something I probably thought along the way but didn't
type as I was trying to show the transformation for at least the review)
John
> + }
> + return true;
> + }
> + }
> + return false;
> +}
> +
> +
> int
> virStoragePoolSourceFindDuplicate(virConnectPtr conn,
> virStoragePoolObjListPtr pools,
> @@ -2390,17 +2412,8 @@ virStoragePoolSourceFindDuplicate(virConnectPtr conn,
> case VIR_STORAGE_POOL_ISCSI:
> matchpool = virStoragePoolSourceFindDuplicateDevices(pool, def);
> if (matchpool) {
> - if (matchpool->def->source.nhost == 1 &&
def->source.nhost == 1) {
> - if (STREQ(matchpool->def->source.hosts[0].name,
def->source.hosts[0].name)) {
> - if ((matchpool->def->source.initiator.iqn) &&
(def->source.initiator.iqn)) {
> - if (STREQ(matchpool->def->source.initiator.iqn,
def->source.initiator.iqn))
> - break;
> - matchpool = NULL;
> - }
> - break;
> - }
> - }
> - matchpool = NULL;
> + if (!matchISCSISource(matchpool, def))
> + matchpool = NULL;
> }
> break;
> case VIR_STORAGE_POOL_FS:
ACK if you rename the function and remove the redundant if.
Peter