[libvirt] [PATCH 10/11] util: Introduce virGetFCHostNameByFabricWWN

[John Ferlan]

On 01/02/2017 09:51 AM, Ján Tomko wrote:
> On Fri, Nov 18, 2016 at 09:26:36AM -0500, John Ferlan wrote:
>> Create a utility routine in order to read the scsi_host fabric_name files
>> looking for a match to a passed fabric_name
>>
>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>> ---
>> src/libvirt_private.syms |  1 +
>> src/util/virutil.c       | 86
>> ++++++++++++++++++++++++++++++++++++++++--------
>> src/util/virutil.h       |  4 +++
>> 3 files changed, 78 insertions(+), 13 deletions(-)
>>
> 
>> diff --git a/src/util/virutil.c b/src/util/virutil.c
>> index a135819..fb72f2d 100644
>> --- a/src/util/virutil.c
>> +++ b/src/util/virutil.c
>> @@ -2166,6 +2166,18 @@ virManageVport(const int parent_host,
>>     return ret;
>> }
>>
>> +# define READ_WWN(wwn_path, buf)                      \
> 
> This macro either jumps to a label or alters a third variable.
> I don't think its scope should extend one function.
> 
>> +    do {                                              \
> 
>> +        if (virFileReadAll(wwn_path, 1024, &buf) < 0) \
>> +            goto cleanup;                             \
> 
> These two lines can be split out and with the STR{,N}EQ included,
> this macro can be COMPARE_WWN, removing the need for changing
> p. Or turned into a more complex function like virUSBSysReadFile.
> 

Not quite sure why it's felt 'p' wouldn't need changing. On my test
system the "port_name", "node_name", and "fabric_wwn" files each have
"0x######\n", but the stored/XML format will be ######

Maybe it's just late and I'm overthinking it.

>> +        if ((p = strchr(buf, '\n')))                  \
>> +            *p = '\0';                                \
>> +        if (STRPREFIX(buf, "0x"))                     \
>> +            p = buf + strlen("0x");                   \
>> +        else                                          \
>> +            p = buf;                                  \
>> +    } while (0)
>> +
>> /* virGetFCHostNameByWWN:
>>  *
>>  * Iterate over the sysfs tree to get FC host name (e.g. host5)
> 
> The rest looks good to me.
> 

So I went with a function:

/* virReadCompareWWN
 * @prefix: path to the wwn file
 * @d_name: name of the current directory
 * @f_name: file name to read
 *
 * Read/compare the on-disk file with the passed wwn value.
 *
 * Returns:
 *   -1 : Error
 *    0 : No match
 *    1 : Match
 */
static int
virReadCompareWWN(const char *prefix,
                  const char *d_name,
                  const char *f_name,
                  const char *wwn)
{
    char *path;
    char *buf = NULL;
    char *p;
    int ret = -1;

    if (virAsprintf(&path, "%s/%s/%s", prefix, d_name, f_name) < 0)
        return -1;

    if (!virFileExists(path)) {
        ret = 0;
        goto cleanup;
    }

    if (virFileReadAll(path, 1024, &buf) < 0)
        goto cleanup;

    if ((p = strchr(buf, '\n')))
        *p = '\0';
    if (STRPREFIX(buf, "0x"))
        p = buf + strlen("0x");
    else
        p = buf;

    if (STRNEQ(wwn, p))
        ret = 0;
    else
        ret = 1;

 cleanup:
    VIR_FREE(path);
    VIR_FREE(buf);

    return ret;
}

It should be obvious what the callers do, but again I can post an
updated series as long as you're generally fine with this.

Tks -

John