support format "127.0.0.1:1234, 127.0.0.2:5678", trim blank, tab

[cuipingxu]

int redisClusterSetOptionAddNodes(redisClusterContext *cc, const char *addrs) {
int ret;
sds *address = NULL;
int address_count = 0;
int i;

if (cc == NULL) {
    return REDIS_ERR;
}

address = sdssplitlen(addrs, strlen(addrs), CLUSTER_ADDRESS_SEPARATOR,
                      strlen(CLUSTER_ADDRESS_SEPARATOR), &address_count);
if (address == NULL) {
    __redisClusterSetError(cc, REDIS_ERR_OOM, "Out of memory");
    return REDIS_ERR;
}

if (address_count <= 0) {
    __redisClusterSetError(cc, REDIS_ERR_OTHER,
                           "invalid server addresses (example format: "
                           "127.0.0.1:1234,127.0.0.2:5678)");
    sdsfreesplitres(address, address_count);
    return REDIS_ERR;
}

for (i = 0; i < address_count; i++) {
sdstrim(address[i],"\t ");  // support format "127.0.0.1:1234, 127.0.0.2:5678",  trim blank, tab
    ret = redisClusterSetOptionAddNode(cc, address[i]);
    if (ret != REDIS_OK) {
        sdsfreesplitres(address, address_count);
        return REDIS_ERR;
    }
}

sdsfreesplitres(address, address_count);

return REDIS_OK;

}

[zuiderkwast]

This is a C library. I think it's OK to require a strict format without flexible spaces in the user input. OTOH, it's only one line added, so not a lot of complexity. What do other people think? (@bjosv, @andreasgerstmayr, etc.)

We'd need to use the string returned by sdstrim though since it may realloc the memory, so

        address[i] = sdstrim(address[i], "\t ");

[bjosv]

This is a C library. I think it's OK to require a strict format without flexible spaces in the user input. OTOH, it's only one line added, so not a lot of complexity. What do other people think? (@bjosv, @andreasgerstmayr, etc.)

I think it would be ok to trim, but at the same time this could easily be done by the user of this library.
A change like this would affect almost all connect functions so this needs to be addressed in the docs.
@cuipingxu what is your use case, do you get the addresses directly from a config?

[cuipingxu]

There is no detailed description for the api.
it is easy to set wrong format of addresses using the api 'redisClusterSetOptionAddNodes'，

[cuipingxu]

suggest adding trim or detailed description for the api. @bjosv

[bjosv]

Thanks for the clarification @cuipingxu. This need to be fixed.

[andreasgerstmayr]

suggest adding trim or detailed description for the api. @bjosv

I'd vote for adding it to the docs instead of adding sdstrim to all connect functions. imho it's reasonable for a library to ask for user input in the correct format.

[zuiderkwast]

The only place it's documented is https://github.com/Nordix/hiredis-cluster#connecting right? It's only documented by an example.

If we add sdstrim, it'd be only to the functions that handle the comma-separated list IMO. Is it more than this one function? If it's only in this function, I think we can do it, but otherwise not.

Whether we add sdstrim or not, we could add some syntactic validation of the addresses and ports here. That could also catch mistakes with spaces.