On 04/20/2015 10:31 AM, Peter Krempa wrote:
> On Sun, Apr 19, 2015 at 20:49:11 -0400, John Ferlan wrote:
>> In order to assist with existing pool source hostname validation against
>> an incoming pool source definition, we need a way to validate that the
>> resolved hostname provided for the pool doesn't match some existing
>> pool; otherwise, we'd have a scenario where two storage pools could be
>> looking at the same data.
>>
>> Search through all the existing pools resolved names against the proposed
>> definitions resolved hostnames - if any match, we return true; otherwise,
>> false is returned
>>
>> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
>> ---
>> src/libvirt_private.syms | 1 +
>> src/util/virutil.c | 106 +++++++++++++++++++++++++++++++++++++++++++++++
>> src/util/virutil.h | 2 +
>> 3 files changed, 109 insertions(+)
>>
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index 5ba9635..a5f4d7f 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -2323,6 +2323,7 @@ virIndexToDiskName;
>> virIsCapableFCHost;
>> virIsCapableVport;
>> virIsDevMapperDevice;
>> +virIsSameHostnameInfo;
>> virIsSUID;
>> virIsValidHostname;
>> virManageVport;
>> diff --git a/src/util/virutil.c b/src/util/virutil.c
>> index f6cc9af..c152b8c 100644
>> --- a/src/util/virutil.c
>> +++ b/src/util/virutil.c
>> @@ -739,6 +739,112 @@ virIsValidHostname(const char *hostname)
>> }
>>
>>
>> +/*
>> + * virIsSameHostnameInfo:
>
> The hostname is not same as the STREQ check is not enough as explained
> below. virIsSameHost perhaps?
>
Sure - Just figured that since I was using the 'getaddrinfo' and
'getnameinfo' that I should incorporate the 'name' and 'info' into the
API name
>> + *
>> + * Take two hostname representations and compare their 'getnameinfo' results
>> + * to ensure that they differ. This will be used by the network storage pool
>> + * validation to ensure an incoming definition doesn't match some existing
>> + * pool definition just because someone tried to change the hostname string
>> + * representation. For example, "localhost" and "127.0.0.1" would fail the
>> + * plain STREQ check, but they essentially are the same host, so the incoming
>> + * definition should be rejected since a pool for the host already exists.
>
> This function is under the genric utils section so the docs should
> explain its general usage.
>
Ok - I'll adjust
>> + *
>> + * @poolhostname: String stored as the pool's hostname
>> + * @defhostname: String to be used by the def's hostname
>
> Same here, the variable naming and this comment doesn't hint that the
> function is universal.
>
yep
>> + *
>> + * Returns true if they are the same, false if not
>
> "may report libvirt error if false is returned" ... but see below [1]
>
>> + */
>> +bool
>> +virIsSameHostnameInfo(const char *poolhostname,
>> + const char *defhostname)
>> +{
>> + int r;
>> + struct addrinfo poolhints, *poolinfo = NULL, *poolcur;
>> + struct addrinfo defhints, *definfo = NULL, *defcur;
>
>
> The hints argument for getaddrinfo is const so you don't neet two
> separate ones [2] ...
>
Right.
>
>> + char poolhost[NI_MAXHOST];
>> + char defhost[NI_MAXHOST];
>
> These take up 2KiB on the stack, wouldn't it be worth allocate them on
> the heap?
>
OK - I'll adjust
>> +
>> + memset(&poolhints, 0, sizeof(poolhints));
>> + poolhints.ai_family = AF_UNSPEC;
>> + poolhints.ai_protocol = IPPROTO_TCP;
>> +
>> + if ((r = getaddrinfo(poolhostname, NULL, &poolhints, &poolinfo)) != 0) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("IP address lookup for poolhostname '%s' failed: %s"),
>> + poolhostname, gai_strerror(r));
>
> [1] Since you are using this function to check that the addresses and
> the 'false' case is when you define the pool, every single error
> reported by this function will just pollute logs. Additionally the
> function that is calling this is not resetting the error, so you might
> get an inaccurate error if something forgets to set error later.
>
I struggled with this too - trying to have less change, but I've
adjusted the calls in my branch to handle the error.
>> + return false;
>> + }
>> +
>> + if (!poolinfo) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("No IP address for poolhostname '%s' found"),
>> + poolhostname);
>> + return false;
>> + }
>> +
>> + memset(&defhints, 0, sizeof(defhints));
>> + defhints.ai_family = AF_UNSPEC;
>> + defhints.ai_protocol = IPPROTO_TCP;
>
> [2] ... and can avoid one of these.
>
>> +
>> + if ((r = getaddrinfo(defhostname, NULL, &defhints, &definfo)) != 0) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("IP address lookup for defhostname '%s' failed: %s"),
>> + defhostname, gai_strerror(r));
>> + goto cleanup;
>> + }
>> +
>> + if (!definfo) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("No IP address for defhostname '%s' found"),
>> + defhostname);
>> + goto cleanup;
>> + }
>> +
>> + for (poolcur = poolinfo; poolcur; poolcur = poolcur->ai_next) {
>> +
>> + if ((r = getnameinfo(poolcur->ai_addr, poolcur->ai_addrlen,
>> + poolhost, sizeof(poolhost), NULL, 0,
>> + NI_NUMERICHOST)) != 0) {
>
> Instead of formatting the address as a string you can use
> virSocketAddrEqual
>
These aren't virSocketAddr's... It seems to me the virSocketAddr API's
are designed to manage a numeric representation of a name - which isn't
a given from the XML
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("Formatting IP address for poolhost '%s' "
>> + "failed: %s"),
>> + poolhostname, gai_strerror(r));
>> + goto cleanup;
>> + }
>> +
>> +
>> + for (defcur = definfo; defcur; defcur = defcur->ai_next) {
>> + if ((r = getnameinfo(defcur->ai_addr, defcur->ai_addrlen,
>> + defhost, sizeof(defhost), NULL, 0,
>> + NI_NUMERICHOST)) != 0) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("Formatting IP address for defhost '%s' "
>> + "failed: %s"),
>> + defhostname, gai_strerror(r));
>> + goto cleanup;
>> + }
>> +
>> + if (poolcur->ai_family != defcur->ai_family)
>> + continue;
>> +
>> + if (STREQ(poolhost, defhost)) {
>> + freeaddrinfo(poolinfo);
>> + freeaddrinfo(definfo);
>> + return true;
>> + }
>> + }
>> + }
>> +
>> + cleanup:
>> + if (poolinfo)
>> + freeaddrinfo(poolinfo);
>> + if (definfo)
>> + freeaddrinfo(definfo);
>> + return false;
>> +}
>> +
>> +
>> char *
>> virGetUserDirectory(void)
>> {
>> diff --git a/src/util/virutil.h b/src/util/virutil.h
>> index 73ad147..5a84411 100644
>> --- a/src/util/virutil.h
>> +++ b/src/util/virutil.h
>
> Storing this in virsocketaddr.[ch] would be better idea IMO.
>
>> @@ -132,6 +132,8 @@ static inline int pthread_sigmask(int how,
>>
>> char *virGetHostname(void);
>> bool virIsValidHostname(const char *hostname) ATTRIBUTE_NONNULL(1);
>> +bool virIsSameHostnameInfo(const char *poolhostname, const char *defhostname)
>> + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
>>
>> char *virGetUserDirectory(void);
>> char *virGetUserDirectoryByUID(uid_t uid);
>
> Resolving the hostname just for the sake of comparing the addresses
> seems a bit overkill, but I can't suggest any better option.
>
I understand/agree - this is one of those bugs I say - well gee don't do
that if it hurts. But then again, by not checking that allows more than
one pool to look at the same target which could have some interesting
repercussions.
> At any rate, I'd like to see a version that uses the helpers in
> virsocketaddr.c that would remove a lot of the open coded stuff.
>
I don't think this sequence belongs in virsocketaddr as they don't
manage virSocketAddr's and because it's handling names and addresses as
strings.
I'll wait to repost with the hopes I'll get some feedback/thoughts or
complaints about what I've already written...
John
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list