7:49 p.m.
https://bugzilla.redhat.com/show_bug.cgi?id=1171984
As a followon to a recent series to conslidate the various network
source host checks into a single API, this series of patches takes the
next steps.
First off existing code assumed the provided "<host name='%s'>"
string
resolved to a valid IP Address; however, that's not necessarily the case.
So rather than assume the path is valid during the various networked
pool startup, open, mount, etc API's check to make sure the host name
can be resolved. More than like the processing was going to fail anyway,
so failing a bit sooner and with a message indicating the problem might
help someone resolve it.
Second now that we hope the running pools are using a resolved name,
check the new/incoming definitions to make sure that their host name
strings do not duplicate an existing/running pool. The existing check
only compares the strings for equality, but with networks a name could
be an IP Address by number (IPv4 or IPv6) or a name that would need to
be resolved. If that name resolves to the same IP Address already running,
then we fail the attempted new pool definition.
John Ferlan (7):
virutil: Introduce virIsValidHostname
iscsi: Check for validity of pool source hostname
netfs: Check for validity of pool source hostname
gluster: Check for validity of pool source hostname
sheepdog: Check for validity of pool source hostname
util: Introduce virIsSameHostnameInfo
storage: Check for duplicate host for incoming pool def
src/conf/storage_conf.c | 11 ++-
src/libvirt_private.syms | 2 +
src/storage/storage_backend_fs.c | 2 +
src/storage/storage_backend_gluster.c | 3 +
src/storage/storage_backend_iscsi.c | 7 ++
src/storage/storage_backend_sheepdog.c | 36 +++++---
src/util/virutil.c | 155 +++++++++++++++++++++++++++++++++
src/util/virutil.h | 3 +
8 files changed, 205 insertions(+), 14 deletions(-)
--
2.1.0
7:49 p.m.
New subject: [libvirt] [PATCH 1/7] virutil: Introduce virIsValidHostname
Similar to virGetHostname, but this time taking a parameter which is a
hostname or ipaddress from a <source ... <host name ='%s'.../>... />
XML property and validating that the name can be resolved.
Return true or false depending on whether we can ascertain the hostname
address from calls to 'getnameinfo' and 'getaddrinfo'. Subsequent patches
will be validating a proposed pool hostname definition against existing
pool hostnames to ensure they are not the same hostname (and thus having
two pools looking at the same data)
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/libvirt_private.syms | 1 +
src/util/virutil.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++++
src/util/virutil.h | 1 +
3 files changed, 51 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 8c37303..5ba9635 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2324,6 +2324,7 @@ virIsCapableFCHost;
virIsCapableVport;
virIsDevMapperDevice;
virIsSUID;
+virIsValidHostname;
virManageVport;
virMemoryLimitIsSet;
virMemoryLimitTruncate;
diff --git a/src/util/virutil.c b/src/util/virutil.c
index 79cdb7a..f6cc9af 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -690,6 +690,55 @@ char *virGetHostname(void)
}
+/*
+ * virIsValidHostname:
+ *
+ * Unlike virGetHostname, this variant of the code receives a hostname
+ * retrieves the getaddrinfo. If the passed hostname can be successfully
+ * and if successfully resolved via getaddrinfo, then success is declared.
+ *
+ * On error in lookup, if an IP Address is not found, or error formatting
+ * the name, an error is generated and a NULL is returned.
+ *
+ * On success, a pointer to a character representation of the numeric IP
+ * Address is returned.
+ *
+ * It is the caller's responsibility to check for a non NULL return and
+ * VIR_FREE the resultant string.
+ */
+bool
+virIsValidHostname(const char *hostname)
+{
+ int r;
+ struct addrinfo hints, *info;
+
+ if (STRPREFIX(hostname, "localhost") ||
+ STREQ(hostname, "127.0.0.1") || STREQ(hostname, "::1"))
+ return true;
+
+ memset(&hints, 0, sizeof(hints));
+ hints.ai_family = AF_UNSPEC;
+ hints.ai_protocol = IPPROTO_TCP;
+
+ if ((r = getaddrinfo(hostname, NULL, &hints, &info)) != 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("IP address lookup for host '%s' failed:
%s"),
+ hostname, gai_strerror(r));
+ return false;
+ }
+
+ if (!info) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("No IP address for host '%s' found"),
+ hostname);
+ return false;
+ }
+ freeaddrinfo(info);
+
+ return true;
+}
+
+
char *
virGetUserDirectory(void)
{
diff --git a/src/util/virutil.h b/src/util/virutil.h
index 55a3bd6..73ad147 100644
--- a/src/util/virutil.h
+++ b/src/util/virutil.h
@@ -131,6 +131,7 @@ static inline int pthread_sigmask(int how,
# endif
char *virGetHostname(void);
+bool virIsValidHostname(const char *hostname) ATTRIBUTE_NONNULL(1);
char *virGetUserDirectory(void);
char *virGetUserDirectoryByUID(uid_t uid);
--
2.1.0
8:40 a.m.
New subject: [libvirt] [PATCH 1/7] virutil: Introduce virIsValidHostname
On Sun, Apr 19, 2015 at 20:49:06 -0400, John Ferlan wrote:
Similar to virGetHostname, but this time taking a parameter which is
a
hostname or ipaddress from a <source ... <host name ='%s'.../>... />
XML property and validating that the name can be resolved.
Return true or false depending on whether we can ascertain the hostname
address from calls to 'getnameinfo' and 'getaddrinfo'. Subsequent
patches
will be validating a proposed pool hostname definition against existing
pool hostnames to ensure they are not the same hostname (and thus having
two pools looking at the same data)
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/libvirt_private.syms | 1 +
src/util/virutil.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++++
src/util/virutil.h | 1 +
3 files changed, 51 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 8c37303..5ba9635 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2324,6 +2324,7 @@ virIsCapableFCHost;
virIsCapableVport;
virIsDevMapperDevice;
virIsSUID;
+virIsValidHostname;
virManageVport;
virMemoryLimitIsSet;
virMemoryLimitTruncate;
diff --git a/src/util/virutil.c b/src/util/virutil.c
index 79cdb7a..f6cc9af 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -690,6 +690,55 @@ char *virGetHostname(void)
}
+/*
+ * virIsValidHostname:
+ *
+ * Unlike virGetHostname, this variant of the code receives a hostname
+ * retrieves the getaddrinfo. If the passed hostname can be successfully
+ * and if successfully resolved via getaddrinfo, then success is declared.
+ *
+ * On error in lookup, if an IP Address is not found, or error formatting
+ * the name, an error is generated and a NULL is returned.
+ *
+ * On success, a pointer to a character representation of the numeric IP
+ * Address is returned.
+ *
+ * It is the caller's responsibility to check for a non NULL return and
+ * VIR_FREE the resultant string.
This comment does not describe how this function works.
+ */
+bool
+virIsValidHostname(const char *hostname)
+{
+ int r;
+ struct addrinfo hints, *info;
+
+ if (STRPREFIX(hostname, "localhost") ||
This certainly is not a good idea:
$ host localhost.pipo.sk
localhost.pipo.sk has address 46.255.230.242
+ STREQ(hostname, "127.0.0.1") || STREQ(hostname,
"::1"))
+ return true;
+
+ memset(&hints, 0, sizeof(hints));
+ hints.ai_family = AF_UNSPEC;
+ hints.ai_protocol = IPPROTO_TCP;
+
+ if ((r = getaddrinfo(hostname, NULL, &hints, &info)) != 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("IP address lookup for host '%s' failed:
%s"),
+ hostname, gai_strerror(r));
+ return false;
+ }
+
+ if (!info) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("No IP address for host '%s' found"),
+ hostname);
+ return false;
+ }
+ freeaddrinfo(info);
+
+ return true;
+}
+
+
Ghm, this seems a bit too excesive IMO, let see how it's used.
Peter
9:34 a.m.
New subject: [libvirt] [PATCH 1/7] virutil: Introduce virIsValidHostname
On Mon, Apr 20, 2015 at 15:40:32 +0200, Peter Krempa wrote:
On Sun, Apr 19, 2015 at 20:49:06 -0400, John Ferlan wrote:
> Similar to virGetHostname, but this time taking a parameter which is a
> hostname or ipaddress from a <source ... <host name ='%s'.../>...
/>
> XML property and validating that the name can be resolved.
>
> Return true or false depending on whether we can ascertain the hostname
> address from calls to 'getnameinfo' and 'getaddrinfo'. Subsequent
patches
> will be validating a proposed pool hostname definition against existing
> pool hostnames to ensure they are not the same hostname (and thus having
> two pools looking at the same data)
>
> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
> ---
> src/libvirt_private.syms | 1 +
> src/util/virutil.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++++
> src/util/virutil.h | 1 +
> 3 files changed, 51 insertions(+)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 8c37303..5ba9635 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2324,6 +2324,7 @@ virIsCapableFCHost;
> virIsCapableVport;
> virIsDevMapperDevice;
> virIsSUID;
> +virIsValidHostname;
> virManageVport;
> virMemoryLimitIsSet;
> virMemoryLimitTruncate;
> diff --git a/src/util/virutil.c b/src/util/virutil.c
> index 79cdb7a..f6cc9af 100644
> --- a/src/util/virutil.c
> +++ b/src/util/virutil.c
> @@ -690,6 +690,55 @@ char *virGetHostname(void)
> }
>
>
> +/*
> + * virIsValidHostname:
> + *
> + * Unlike virGetHostname, this variant of the code receives a hostname
> + * retrieves the getaddrinfo. If the passed hostname can be successfully
> + * and if successfully resolved via getaddrinfo, then success is declared.
> + *
> + * On error in lookup, if an IP Address is not found, or error formatting
> + * the name, an error is generated and a NULL is returned.
> + *
> + * On success, a pointer to a character representation of the numeric IP
> + * Address is returned.
> + *
> + * It is the caller's responsibility to check for a non NULL return and
> + * VIR_FREE the resultant string.
This comment does not describe how this function works.
> + */
> +bool
> +virIsValidHostname(const char *hostname)
As a hindsight from reviewing 6/7. This function should also be in
virsocketaddr.c
Peter
10:15 a.m.
New subject: [libvirt] [PATCH 1/7] virutil: Introduce virIsValidHostname
On 04/20/2015 10:34 AM, Peter Krempa wrote:
On Mon, Apr 20, 2015 at 15:40:32 +0200, Peter Krempa wrote:
> On Sun, Apr 19, 2015 at 20:49:06 -0400, John Ferlan wrote:
>> Similar to virGetHostname, but this time taking a parameter which is a
>> hostname or ipaddress from a <source ... <host name
='%s'.../>... />
>> XML property and validating that the name can be resolved.
>>
>> Return true or false depending on whether we can ascertain the hostname
>> address from calls to 'getnameinfo' and 'getaddrinfo'. Subsequent
patches
>> will be validating a proposed pool hostname definition against existing
>> pool hostnames to ensure they are not the same hostname (and thus having
>> two pools looking at the same data)
>>
>> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
>> ---
>> src/libvirt_private.syms | 1 +
>> src/util/virutil.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++++
>> src/util/virutil.h | 1 +
>> 3 files changed, 51 insertions(+)
>>
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index 8c37303..5ba9635 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -2324,6 +2324,7 @@ virIsCapableFCHost;
>> virIsCapableVport;
>> virIsDevMapperDevice;
>> virIsSUID;
>> +virIsValidHostname;
>> virManageVport;
>> virMemoryLimitIsSet;
>> virMemoryLimitTruncate;
>> diff --git a/src/util/virutil.c b/src/util/virutil.c
>> index 79cdb7a..f6cc9af 100644
>> --- a/src/util/virutil.c
>> +++ b/src/util/virutil.c
>> @@ -690,6 +690,55 @@ char *virGetHostname(void)
>> }
>>
>>
>> +/*
>> + * virIsValidHostname:
>> + *
>> + * Unlike virGetHostname, this variant of the code receives a hostname
>> + * retrieves the getaddrinfo. If the passed hostname can be successfully
>> + * and if successfully resolved via getaddrinfo, then success is declared.
>> + *
>> + * On error in lookup, if an IP Address is not found, or error formatting
>> + * the name, an error is generated and a NULL is returned.
>> + *
>> + * On success, a pointer to a character representation of the numeric IP
>> + * Address is returned.
>> + *
>> + * It is the caller's responsibility to check for a non NULL return and
>> + * VIR_FREE the resultant string.
>
> This comment does not describe how this function works.
>
Err... Oh yeah - I changed how I did things, but forget to update the
comment, <sigh>
>> + */
>> +bool
>> +virIsValidHostname(const char *hostname)
As a hindsight from reviewing 6/7. This function should also be in
virsocketaddr.c
hmmm.. yes I see.. Guess I got hung up on "virSocketAddr..." and didn't
focus as closely on the implementation where virSocketAddrParse can take
NULL as the first parameter... Guess, that means patches 2-5 can just be
called as:
if (virSocketAddrParse(NULL, pool->def->source.hosts[0].name,
AF_UNSPEC) < 0)
Tks -
John
2:25 p.m.
New subject: [libvirt] [PATCH 1/7] virutil: Introduce virIsValidHostname
> As a hindsight from reviewing 6/7. This function should also be
in
> virsocketaddr.c
>
>
hmmm.. yes I see.. Guess I got hung up on "virSocketAddr..." and didn't
focus as closely on the implementation where virSocketAddrParse can take
NULL as the first parameter... Guess, that means patches 2-5 can just be
called as:
if (virSocketAddrParse(NULL, pool->def->source.hosts[0].name,
AF_UNSPEC) < 0)
Without actually trying it - seemed like a good idea; however, the
virSocketAddrParse uses/sets "hints.ai_flags = AI_NUMERICHOST;" thus
it requires a numeric value and not one that could be a name or a
number, so it seems this particular code cannot use it.
I really see those virSocketAddr* API's as different, very specific
to supporting the network socket's and socket address formats; whereas,
this code will take a string representation of either the name or
the number as provided in the XML and validate it.
I don't think this set of API's belongs there as it's not manipulating
virSocketAddr's.
So, I'll change the function intro to:
* Unlike virGetHostname, this variant of the code receives a hostname and
* retrieves the getaddrinfo. If the passed hostname can be successfully
* resolved via getaddrinfo, then return true; otherwise, if the hostname
* cannot be resolved for any reason, return false.
and
remove the localhost specific checking
and adjust the commit message to remove the 'getnameinfo' reference.
John
3:36 a.m.
New subject: [libvirt] [PATCH 1/7] virutil: Introduce virIsValidHostname
On Mon, Apr 20, 2015 at 15:25:29 -0400, John Ferlan wrote:
>> As a hindsight from reviewing 6/7. This function should also be in
>> virsocketaddr.c
>>
>>
>
>
> hmmm.. yes I see.. Guess I got hung up on "virSocketAddr..." and
didn't
> focus as closely on the implementation where virSocketAddrParse can take
> NULL as the first parameter... Guess, that means patches 2-5 can just be
> called as:
>
> if (virSocketAddrParse(NULL, pool->def->source.hosts[0].name,
> AF_UNSPEC) < 0)
>
>
Without actually trying it - seemed like a good idea; however, the
virSocketAddrParse uses/sets "hints.ai_flags = AI_NUMERICHOST;" thus
it requires a numeric value and not one that could be a name or a
number, so it seems this particular code cannot use it.
Well, you still can add a new helper that will allow to use your
approach.
I really see those virSocketAddr* API's as different, very specific
to supporting the network socket's and socket address formats; whereas,
this code will take a string representation of either the name or
the number as provided in the XML and validate it.
We still can make it a more universal helper for address conversions.
I don't think this set of API's belongs there as it's not manipulating
virSocketAddr's.
The general utils file used to be a dumping place for all the various
helpers, but now we are usually trying to put them together with the
semanticaly similar helpers, or making a group of them to make them
semantically similar. I think it makes sense in this case and the
general utils file is not the right place for this helper.
So, I'll change the function intro to:
* Unlike virGetHostname, this variant of the code receives a hostname and
* retrieves the getaddrinfo. If the passed hostname can be successfully
* resolved via getaddrinfo, then return true; otherwise, if the hostname
* cannot be resolved for any reason, return false.
and
remove the localhost specific checking
and adjust the commit message to remove the 'getnameinfo' reference.
John
Peter
9:58 a.m.
New subject: [libvirt] [PATCH 1/7] virutil: Introduce virIsValidHostname
On Sun, Apr 19, 2015 at 08:49:06PM -0400, John Ferlan wrote:
Similar to virGetHostname, but this time taking a parameter which is
a
hostname or ipaddress from a <source ... <host name ='%s'.../>... />
XML property and validating that the name can be resolved.
Return true or false depending on whether we can ascertain the hostname
address from calls to 'getnameinfo' and 'getaddrinfo'. Subsequent
patches
will be validating a proposed pool hostname definition against existing
pool hostnames to ensure they are not the same hostname (and thus having
two pools looking at the same data)
Why do we need this function? The failure to resolve the host is already handled
by the underlying commands.
I don't think adding all this code just to save us a fork on wrong input is worth it.
+bool
+virIsValidHostname(const char *hostname)
+{
+ int r;
+ struct addrinfo hints, *info;
+
+ if (STRPREFIX(hostname, "localhost") ||
+ STREQ(hostname, "127.0.0.1") || STREQ(hostname, "::1"))
getaddrinfo handles these two numeric representations of localhost just
fine, just as the rest of them (see DO_TEST_LOCALHOST in sockettest.c
for a few interesting examples)
Jan
7:49 p.m.
New subject: [libvirt] [PATCH 2/7] iscsi: Check for validity of pool source hostname
Ensure that the pool that's being started has a source pool hostname
that can be resolved before trying to start an iSCSI session.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/storage/storage_backend_iscsi.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c
index 197d333..958c347 100644
--- a/src/storage/storage_backend_iscsi.c
+++ b/src/storage/storage_backend_iscsi.c
@@ -385,6 +385,13 @@ virStorageBackendISCSIStartPool(virConnectPtr conn,
return -1;
}
+ if (!virIsValidHostname(pool->def->source.hosts[0].name)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("cannot resolve hostname '%s' on this host"),
+ pool->def->source.hosts[0].name);
+ return -1;
+ }
+
if (pool->def->source.ndevice != 1 ||
pool->def->source.devices[0].path == NULL) {
virReportError(VIR_ERR_INTERNAL_ERROR,
--
2.1.0
8:41 a.m.
New subject: [libvirt] [PATCH 2/7] iscsi: Check for validity of pool source hostname
On Sun, Apr 19, 2015 at 20:49:07 -0400, John Ferlan wrote:
Ensure that the pool that's being started has a source pool
hostname
that can be resolved before trying to start an iSCSI session.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/storage/storage_backend_iscsi.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c
index 197d333..958c347 100644
--- a/src/storage/storage_backend_iscsi.c
+++ b/src/storage/storage_backend_iscsi.c
@@ -385,6 +385,13 @@ virStorageBackendISCSIStartPool(virConnectPtr conn,
return -1;
}
+ if (!virIsValidHostname(pool->def->source.hosts[0].name)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("cannot resolve hostname '%s' on this
host"),
+ pool->def->source.hosts[0].name);
+ return -1;
This overwrites the error from virIsValidHostname()
Peter
2:29 p.m.
New subject: [libvirt] [PATCH 2/7] iscsi: Check for validity of pool source hostname
On 04/20/2015 09:41 AM, Peter Krempa wrote:
On Sun, Apr 19, 2015 at 20:49:07 -0400, John Ferlan wrote:
> Ensure that the pool that's being started has a source pool hostname
> that can be resolved before trying to start an iSCSI session.
>
> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
> ---
> src/storage/storage_backend_iscsi.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/src/storage/storage_backend_iscsi.c
b/src/storage/storage_backend_iscsi.c
> index 197d333..958c347 100644
> --- a/src/storage/storage_backend_iscsi.c
> +++ b/src/storage/storage_backend_iscsi.c
> @@ -385,6 +385,13 @@ virStorageBackendISCSIStartPool(virConnectPtr conn,
> return -1;
> }
>
> + if (!virIsValidHostname(pool->def->source.hosts[0].name)) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("cannot resolve hostname '%s' on this
host"),
> + pool->def->source.hosts[0].name);
> + return -1;
This overwrites the error from virIsValidHostname()
Oh right - relic of first change...
Removed the ReportError here
John
7:49 p.m.
New subject: [libvirt] [PATCH 3/7] netfs: Check for validity of pool source hostname
Before attempting to mount the netfs pool, ensure the source pool
hostname can be resolved to something this host knows.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/storage/storage_backend_fs.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
index 521dc70..7f99cb1 100644
--- a/src/storage/storage_backend_fs.c
+++ b/src/storage/storage_backend_fs.c
@@ -408,6 +408,8 @@ virStorageBackendFileSystemMount(virStoragePoolObjPtr pool)
"%s", _("missing source path"));
return -1;
}
+ if (!virIsValidHostname(pool->def->source.hosts[0].name))
+ return -1;
} else {
if (pool->def->source.ndevice != 1) {
virReportError(VIR_ERR_INTERNAL_ERROR,
--
2.1.0
7:49 p.m.
New subject: [libvirt] [PATCH 4/7] gluster: Check for validity of pool source hostname
Prior to attempting to open the gluster connection, let's make sure we
can resolve the source pool hostname.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/storage/storage_backend_gluster.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/storage/storage_backend_gluster.c
b/src/storage/storage_backend_gluster.c
index d2e79bc..e3a8c21 100644
--- a/src/storage/storage_backend_gluster.c
+++ b/src/storage/storage_backend_gluster.c
@@ -96,6 +96,9 @@ virStorageBackendGlusterOpen(virStoragePoolObjPtr pool)
trailing_slash = false;
}
+ if (!virIsValidHostname(pool->def->source.hosts[0].name))
+ return NULL;
+
if (VIR_ALLOC(ret) < 0)
return NULL;
--
2.1.0
8:45 a.m.
New subject: [libvirt] [PATCH 4/7] gluster: Check for validity of pool source hostname
On Sun, Apr 19, 2015 at 20:49:09 -0400, John Ferlan wrote:
Prior to attempting to open the gluster connection, let's make
sure we
can resolve the source pool hostname.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/storage/storage_backend_gluster.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/storage/storage_backend_gluster.c
b/src/storage/storage_backend_gluster.c
index d2e79bc..e3a8c21 100644
--- a/src/storage/storage_backend_gluster.c
+++ b/src/storage/storage_backend_gluster.c
@@ -96,6 +96,9 @@ virStorageBackendGlusterOpen(virStoragePoolObjPtr pool)
trailing_slash = false;
}
+ if (!virIsValidHostname(pool->def->source.hosts[0].name))
+ return NULL;
+
Okay, it might be marginally worth at this point, since gluster is/was
leaking some memory and threads after it was initialized.
On the other hand you'll be resolving the hostname again a few lines
below.
if (VIR_ALLOC(ret) < 0)
return NULL;
Peter
2:39 p.m.
New subject: [libvirt] [PATCH 4/7] gluster: Check for validity of pool source hostname
On 04/20/2015 09:45 AM, Peter Krempa wrote:
On Sun, Apr 19, 2015 at 20:49:09 -0400, John Ferlan wrote:
> Prior to attempting to open the gluster connection, let's make sure we
> can resolve the source pool hostname.
>
> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
> ---
> src/storage/storage_backend_gluster.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/src/storage/storage_backend_gluster.c
b/src/storage/storage_backend_gluster.c
> index d2e79bc..e3a8c21 100644
> --- a/src/storage/storage_backend_gluster.c
> +++ b/src/storage/storage_backend_gluster.c
> @@ -96,6 +96,9 @@ virStorageBackendGlusterOpen(virStoragePoolObjPtr pool)
> trailing_slash = false;
> }
>
> + if (!virIsValidHostname(pool->def->source.hosts[0].name))
> + return NULL;
> +
Okay, it might be marginally worth at this point, since gluster is/was
leaking some memory and threads after it was initialized.
On the other hand you'll be resolving the hostname again a few lines
below.
Which call, glfs_set_volfile_server? It wasn't intuitively obvious...
John
> if (VIR_ALLOC(ret) < 0)
> return NULL;
Peter
3:37 a.m.
New subject: [libvirt] [PATCH 4/7] gluster: Check for validity of pool source hostname
On Mon, Apr 20, 2015 at 15:39:23 -0400, John Ferlan wrote:
On 04/20/2015 09:45 AM, Peter Krempa wrote:
> On Sun, Apr 19, 2015 at 20:49:09 -0400, John Ferlan wrote:
>> Prior to attempting to open the gluster connection, let's make sure we
>> can resolve the source pool hostname.
>>
>> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
>> ---
>> src/storage/storage_backend_gluster.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/src/storage/storage_backend_gluster.c
b/src/storage/storage_backend_gluster.c
>> index d2e79bc..e3a8c21 100644
>> --- a/src/storage/storage_backend_gluster.c
>> +++ b/src/storage/storage_backend_gluster.c
>> @@ -96,6 +96,9 @@ virStorageBackendGlusterOpen(virStoragePoolObjPtr pool)
>> trailing_slash = false;
>> }
>>
>> + if (!virIsValidHostname(pool->def->source.hosts[0].name))
>> + return NULL;
>> +
>
> Okay, it might be marginally worth at this point, since gluster is/was
> leaking some memory and threads after it was initialized.
>
> On the other hand you'll be resolving the hostname again a few lines
> below.
>
Which call, glfs_set_volfile_server? It wasn't intuitively obvious...
glfs_init does all the magic (and leaking)
7:49 p.m.
New subject: [libvirt] [PATCH 5/7] sheepdog: Check for validity of pool source hostname
If there is a pool source hostname provided, let's make sure the name
can be resolved before trying to connect to it via a virCommand sequence.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/storage/storage_backend_sheepdog.c | 36 ++++++++++++++++++++++------------
1 file changed, 23 insertions(+), 13 deletions(-)
diff --git a/src/storage/storage_backend_sheepdog.c
b/src/storage/storage_backend_sheepdog.c
index af15c3b..78caae5 100644
--- a/src/storage/storage_backend_sheepdog.c
+++ b/src/storage/storage_backend_sheepdog.c
@@ -40,9 +40,6 @@ static int virStorageBackendSheepdogRefreshVol(virConnectPtr conn,
virStoragePoolObjPtr pool,
virStorageVolDefPtr vol);
-void virStorageBackendSheepdogAddHostArg(virCommandPtr cmd,
- virStoragePoolObjPtr pool);
-
int
virStorageBackendSheepdogParseNodeInfo(virStoragePoolDefPtr pool,
char *output)
@@ -90,7 +87,7 @@ virStorageBackendSheepdogParseNodeInfo(virStoragePoolDefPtr pool,
return -1;
}
-void
+static int
virStorageBackendSheepdogAddHostArg(virCommandPtr cmd,
virStoragePoolObjPtr pool)
{
@@ -99,6 +96,8 @@ virStorageBackendSheepdogAddHostArg(virCommandPtr cmd,
if (pool->def->source.nhost > 0) {
if (pool->def->source.hosts[0].name != NULL)
address = pool->def->source.hosts[0].name;
+ if (!virIsValidHostname(address))
+ return -1;
if (pool->def->source.hosts[0].port)
port = pool->def->source.hosts[0].port;
}
@@ -106,6 +105,7 @@ virStorageBackendSheepdogAddHostArg(virCommandPtr cmd,
virCommandAddArgFormat(cmd, "%s", address);
virCommandAddArg(cmd, "-p");
virCommandAddArgFormat(cmd, "%d", port);
+ return 0;
}
static int
@@ -151,7 +151,8 @@ virStorageBackendSheepdogRefreshAllVol(virConnectPtr conn
ATTRIBUTE_UNUSED,
size_t i;
virCommandPtr cmd = virCommandNewArgList(COLLIE, "vdi", "list",
"-r", NULL);
- virStorageBackendSheepdogAddHostArg(cmd, pool);
+ if (virStorageBackendSheepdogAddHostArg(cmd, pool) < 0)
+ goto cleanup;
virCommandSetOutputBuffer(cmd, &output);
if (virCommandRun(cmd, NULL) < 0)
goto cleanup;
@@ -196,7 +197,8 @@ virStorageBackendSheepdogRefreshPool(virConnectPtr conn
ATTRIBUTE_UNUSED,
virCommandPtr cmd;
cmd = virCommandNewArgList(COLLIE, "node", "info",
"-r", NULL);
- virStorageBackendSheepdogAddHostArg(cmd, pool);
+ if (virStorageBackendSheepdogAddHostArg(cmd, pool) < 0)
+ goto cleanup;
virCommandSetOutputBuffer(cmd, &output);
if (virCommandRun(cmd, NULL) < 0)
goto cleanup;
@@ -218,13 +220,16 @@ virStorageBackendSheepdogDeleteVol(virConnectPtr conn
ATTRIBUTE_UNUSED,
virStorageVolDefPtr vol,
unsigned int flags)
{
+ int ret = -1;
virCheckFlags(0, -1);
virCommandPtr cmd = virCommandNewArgList(COLLIE, "vdi", "delete",
vol->name, NULL);
- virStorageBackendSheepdogAddHostArg(cmd, pool);
- int ret = virCommandRun(cmd, NULL);
+ if (virStorageBackendSheepdogAddHostArg(cmd, pool) < 0)
+ goto cleanup;
+ ret = virCommandRun(cmd, NULL);
+ cleanup:
virCommandFree(cmd);
return ret;
}
@@ -275,7 +280,8 @@ virStorageBackendSheepdogBuildVol(virConnectPtr conn,
cmd = virCommandNewArgList(COLLIE, "vdi", "create", vol->name,
NULL);
virCommandAddArgFormat(cmd, "%llu", vol->target.capacity);
- virStorageBackendSheepdogAddHostArg(cmd, pool);
+ if (virStorageBackendSheepdogAddHostArg(cmd, pool) < 0)
+ goto cleanup;
if (virCommandRun(cmd, NULL) < 0)
goto cleanup;
@@ -355,11 +361,12 @@ virStorageBackendSheepdogRefreshVol(virConnectPtr conn
ATTRIBUTE_UNUSED,
virStoragePoolObjPtr pool,
virStorageVolDefPtr vol)
{
- int ret;
+ int ret = -1;
char *output = NULL;
virCommandPtr cmd = virCommandNewArgList(COLLIE, "vdi", "list",
vol->name, "-r", NULL);
- virStorageBackendSheepdogAddHostArg(cmd, pool);
+ if (virStorageBackendSheepdogAddHostArg(cmd, pool) < 0)
+ goto cleanup;
virCommandSetOutputBuffer(cmd, &output);
ret = virCommandRun(cmd, NULL);
@@ -391,14 +398,17 @@ virStorageBackendSheepdogResizeVol(virConnectPtr conn
ATTRIBUTE_UNUSED,
unsigned long long capacity,
unsigned int flags)
{
+ int ret = -1;
virCheckFlags(0, -1);
virCommandPtr cmd = virCommandNewArgList(COLLIE, "vdi", "resize",
vol->name, NULL);
virCommandAddArgFormat(cmd, "%llu", capacity);
- virStorageBackendSheepdogAddHostArg(cmd, pool);
- int ret = virCommandRun(cmd, NULL);
+ if (virStorageBackendSheepdogAddHostArg(cmd, pool) < 0)
+ goto cleanup;
+ ret = virCommandRun(cmd, NULL);
+ cleanup:
virCommandFree(cmd);
return ret;
--
2.1.0
7:49 p.m.
New subject: [libvirt] [PATCH 6/7] util: Introduce virIsSameHostnameInfo
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(a)redhat.com>
---
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:
+ *
+ * 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.
+ *
+ * @poolhostname: String stored as the pool's hostname
+ * @defhostname: String to be used by the def's hostname
+ *
+ * Returns true if they are the same, false if not
+ */
+bool
+virIsSameHostnameInfo(const char *poolhostname,
+ const char *defhostname)
+{
+ int r;
+ struct addrinfo poolhints, *poolinfo = NULL, *poolcur;
+ struct addrinfo defhints, *definfo = NULL, *defcur;
+ char poolhost[NI_MAXHOST];
+ char defhost[NI_MAXHOST];
+
+ 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));
+ 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;
+
+ 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) {
+ 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
@@ -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);
--
2.1.0
9:31 a.m.
New subject: [libvirt] [PATCH 6/7] util: Introduce virIsSameHostnameInfo
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(a)redhat.com>
---
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?
+ *
+ * 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.
+ *
+ * @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.
+ *
+ * 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] ...
+ 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?
+
+ 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.
+ 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
+ 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.
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.
Peter
6:30 p.m.
New subject: [libvirt] [PATCH 6/7] util: Introduce virIsSameHostnameInfo
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(a)redhat.com>
> ---
> 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
7:49 p.m.
New subject: [libvirt] [PATCH 7/7] storage: Check for duplicate host for incoming pool def
https://bugzilla.redhat.com/show_bug.cgi?id=1171984
Use the new virIsSameHostnameInfo API to determine whether the proposed
storage pool definition matches the existing storage pool definition
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/storage_conf.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 4852dfb..c1bc242 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -2415,7 +2415,16 @@ virStoragePoolSourceMatchSingleHost(virStoragePoolSourcePtr
poolsrc,
if (poolsrc->hosts[0].port != defsrc->hosts[0].port)
return false;
- return STREQ(poolsrc->hosts[0].name, defsrc->hosts[0].name);
+ if (STRNEQ(poolsrc->hosts[0].name, defsrc->hosts[0].name)) {
+ /* Matching just a name isn't reliable as someone could provide
+ * the name for one pool and the IP Address for another pool, so
+ * for a "true" check we must compare the resolved name, but
that's
+ * expensive, so only do this check if using different names
+ */
+ return virIsSameHostnameInfo(poolsrc->hosts[0].name,
+ defsrc->hosts[0].name);
+ }
+ return true;
}
--
2.1.0
9:34 a.m.
New subject: [libvirt] [PATCH 7/7] storage: Check for duplicate host for incoming pool def
On Sun, Apr 19, 2015 at 20:49:12 -0400, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1171984
Use the new virIsSameHostnameInfo API to determine whether the proposed
storage pool definition matches the existing storage pool definition
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/storage_conf.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 4852dfb..c1bc242 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -2415,7 +2415,16 @@ virStoragePoolSourceMatchSingleHost(virStoragePoolSourcePtr
poolsrc,
if (poolsrc->hosts[0].port != defsrc->hosts[0].port)
return false;
- return STREQ(poolsrc->hosts[0].name, defsrc->hosts[0].name);
+ if (STRNEQ(poolsrc->hosts[0].name, defsrc->hosts[0].name)) {
This check is almost worthless. In the normal use case the user won't
try to define the pool with same name twice. On many other cases the
pool def might use different hostnames or so. Using the resolve helper
always might save this condition ....
+ /* Matching just a name isn't reliable as someone could
provide
+ * the name for one pool and the IP Address for another pool, so
+ * for a "true" check we must compare the resolved name, but
that's
+ * expensive, so only do this check if using different names
+ */
... and you wouldn't need to explain why you've done it this way.
+ return virIsSameHostnameInfo(poolsrc->hosts[0].name,
+ defsrc->hosts[0].name);
+ }
+ return true;
}
Peter
10:11 a.m.
New subject: [libvirt] [PATCH 7/7] storage: Check for duplicate host for incoming pool def
On Sun, Apr 19, 2015 at 08:49:12PM -0400, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1171984
Use the new virIsSameHostnameInfo API to determine whether the proposed
storage pool definition matches the existing storage pool definition
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/storage_conf.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 4852dfb..c1bc242 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -2415,7 +2415,16 @@ virStoragePoolSourceMatchSingleHost(virStoragePoolSourcePtr
poolsrc,
if (poolsrc->hosts[0].port != defsrc->hosts[0].port)
return false;
This function is called when parsing the configuration, which should not
depend on host state.
For example, if libvirt is started really early at boot time and the
hostnames cannot be resolved by the DNS yet, they will pass the check
but they will disappear on libvirtd restart.
The hostname->ip pairings are not stable either, so if we do this, I
think it should be done on pool startup, not config parsing.
- return STREQ(poolsrc->hosts[0].name,
defsrc->hosts[0].name);
+ if (STRNEQ(poolsrc->hosts[0].name, defsrc->hosts[0].name)) {
+ /* Matching just a name isn't reliable as someone could provide
+ * the name for one pool and the IP Address for another pool, so
Resolving them is IMHO just as unreliable.
Re: the original bug - is it possible to check that we have connected to
a session with a different hostname than what we requested?
Or just disallow starting two pools with the same targetname, since they
are supposed to be unique?
Jan
11:23 a.m.
New subject: [libvirt] [PATCH 7/7] storage: Check for duplicate host for incoming pool def
On 04/20/2015 11:11 AM, Ján Tomko wrote:
On Sun, Apr 19, 2015 at 08:49:12PM -0400, John Ferlan wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1171984
>
> Use the new virIsSameHostnameInfo API to determine whether the proposed
> storage pool definition matches the existing storage pool definition
>
> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
> ---
> src/conf/storage_conf.c | 11 ++++++++++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
> index 4852dfb..c1bc242 100644
> --- a/src/conf/storage_conf.c
> +++ b/src/conf/storage_conf.c
> @@ -2415,7 +2415,16 @@ virStoragePoolSourceMatchSingleHost(virStoragePoolSourcePtr
poolsrc,
> if (poolsrc->hosts[0].port != defsrc->hosts[0].port)
> return false;
>
This function is called when parsing the configuration, which should not
depend on host state.
For example, if libvirt is started really early at boot time and the
hostnames cannot be resolved by the DNS yet, they will pass the check
but they will disappear on libvirtd restart.
Hmm... the downside of unreliable dependencies.
The hostname->ip pairings are not stable either, so if we do this, I
think it should be done on pool startup, not config parsing.
Right, but by the time we get to pool startup we'd be at the same point
as referenced above - if done early enough at boot time, then it's not
going to fail, but perhaps better than nothing.
I suppose at least moving to startup allows for better error paths since
currently errors can be overwritten or ignored.
> - return STREQ(poolsrc->hosts[0].name,
defsrc->hosts[0].name);
> + if (STRNEQ(poolsrc->hosts[0].name, defsrc->hosts[0].name)) {
> + /* Matching just a name isn't reliable as someone could provide
> + * the name for one pool and the IP Address for another pool, so
Resolving them is IMHO just as unreliable.
Re: the original bug - is it possible to check that we have connected to
a session with a different hostname than what we requested?
What does the connected session hostname have to do with the original
bug? The bug requests checking that an iSCSI pool doesn't use a "<host
name='<some IP Address>'..." for one pool:
<host name='10.66.6.12' port='3260'/>
and a resolved name for another pool on the same host:
<host name='test1' port='3260'/>
Where :
# cat /etc/hosts
10.66.6.12 test1
The bug pointed out iSCSI in particular, but since other pools use the
<source... <host name='%s'.../>... /> XML formatting it seemed
logical
to have them all use the same checks.
Or just disallow starting two pools with the same targetname, since they
are supposed to be unique?
'targetname' as in ?
A 'target.path' per pool would need to be unique, but using the same
target.path into two different networked pools is something that should
work. And the pool target path (/dev/disk/by-path) is used by multiple
iSCSI pools on the same host, so it cannot be used as something unique
per host.
John
3:31 a.m.
New subject: [libvirt] [PATCH 7/7] storage: Check for duplicate host for incoming pool def
On Mon, Apr 20, 2015 at 12:23:25 -0400, John Ferlan wrote:
On 04/20/2015 11:11 AM, Ján Tomko wrote:
> On Sun, Apr 19, 2015 at 08:49:12PM -0400, John Ferlan wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1171984
>>
>> Use the new virIsSameHostnameInfo API to determine whether the proposed
>> storage pool definition matches the existing storage pool definition
>>
>> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
>> ---
>> src/conf/storage_conf.c | 11 ++++++++++-
>> 1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
>> index 4852dfb..c1bc242 100644
>> --- a/src/conf/storage_conf.c
>> +++ b/src/conf/storage_conf.c
>> @@ -2415,7 +2415,16 @@
virStoragePoolSourceMatchSingleHost(virStoragePoolSourcePtr poolsrc,
>> if (poolsrc->hosts[0].port != defsrc->hosts[0].port)
>> return false;
>>
>
> This function is called when parsing the configuration, which should not
> depend on host state.
>
> For example, if libvirt is started really early at boot time and the
> hostnames cannot be resolved by the DNS yet, they will pass the check
> but they will disappear on libvirtd restart.
Hmm... the downside of unreliable dependencies.
Not only unreliable dependencies. The issue might also happen with
factors external to the host. For example by adding a new DNS name for
the "duplicate" host.
>
> The hostname->ip pairings are not stable either, so if we do this, I
> think it should be done on pool startup, not config parsing.
>
Right, but by the time we get to pool startup we'd be at the same point
as referenced above - if done early enough at boot time, then it's not
going to fail, but perhaps better than nothing.
Well, that's actually not true. If the second (duplicate) pool is
starting at one point, the address either can be resolved and deemed
duplicate, or the resolution would fail anyways and the pool would not
be started.
When compared with the approach when defining the pool (or reading back
XML files):
1) if the hostname is not duplicate or not resolvable at the point when
you define the pool but changes to be duplicate later you wouldn't catch
the issue
2) if the pool source becomes duplicate in the life of the host it would
vanish after libvirt restart
Peter
10:02 a.m.
New subject: [libvirt] [PATCH 7/7] storage: Check for duplicate host for incoming pool def
On 04/21/2015 04:31 AM, Peter Krempa wrote:
On Mon, Apr 20, 2015 at 12:23:25 -0400, John Ferlan wrote:
>
>
> On 04/20/2015 11:11 AM, Ján Tomko wrote:
>> On Sun, Apr 19, 2015 at 08:49:12PM -0400, John Ferlan wrote:
>>> https://bugzilla.redhat.com/show_bug.cgi?id=1171984
>>>
>>> Use the new virIsSameHostnameInfo API to determine whether the proposed
>>> storage pool definition matches the existing storage pool definition
>>>
>>> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
>>> ---
>>> src/conf/storage_conf.c | 11 ++++++++++-
>>> 1 file changed, 10 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
>>> index 4852dfb..c1bc242 100644
>>> --- a/src/conf/storage_conf.c
>>> +++ b/src/conf/storage_conf.c
>>> @@ -2415,7 +2415,16 @@
virStoragePoolSourceMatchSingleHost(virStoragePoolSourcePtr poolsrc,
>>> if (poolsrc->hosts[0].port != defsrc->hosts[0].port)
>>> return false;
>>>
>>
>> This function is called when parsing the configuration, which should not
>> depend on host state.
>>
>> For example, if libvirt is started really early at boot time and the
>> hostnames cannot be resolved by the DNS yet, they will pass the check
>> but they will disappear on libvirtd restart.
>
> Hmm... the downside of unreliable dependencies.
Not only unreliable dependencies. The issue might also happen with
factors external to the host. For example by adding a new DNS name for
the "duplicate" host.
>
>>
>> The hostname->ip pairings are not stable either, so if we do this, I
>> think it should be done on pool startup, not config parsing.
>>
>
> Right, but by the time we get to pool startup we'd be at the same point
> as referenced above - if done early enough at boot time, then it's not
> going to fail, but perhaps better than nothing.
Well, that's actually not true. If the second (duplicate) pool is
starting at one point, the address either can be resolved and deemed
duplicate, or the resolution would fail anyways and the pool would not
be started.
When compared with the approach when defining the pool (or reading back
XML files):
1) if the hostname is not duplicate or not resolvable at the point when
you define the pool but changes to be duplicate later you wouldn't catch
the issue
2) if the pool source becomes duplicate in the life of the host it would
vanish after libvirt restart
Working through refactoring and I've reached this point... Started
looking into having the 'startPool' backends make the duplicate pool
checks, but that won't work because by that time the new definition
would have been added to the driver->pools list. Also we don't have the
full driver->pools list passed, just the new definition.
So since the hangup seems to be on the define processing, what if I add
a fourth parameter 'check_active' to virStoragePoolSourceFindDuplicate
(like virStoragePoolObjIsDuplicate), which only does the resolution
checking if we're about to become active? That would mean another round
of checks in storagePoolCreate
Optionally, a new API such as virStoragePoolSourceNetworkCheck (or
whatever name someone prefers) could perform the checks for the
CreateXML and Create paths
John
4:36 a.m.
New subject: [libvirt] [PATCH 7/7] storage: Check for duplicate host for incoming pool def
On Mon, Apr 20, 2015 at 12:23:25PM -0400, John Ferlan wrote:
On 04/20/2015 11:11 AM, Ján Tomko wrote:
> On Sun, Apr 19, 2015 at 08:49:12PM -0400, John Ferlan wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1171984
>>
>> Use the new virIsSameHostnameInfo API to determine whether the proposed
>> storage pool definition matches the existing storage pool definition
>>
>> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
>> ---
>> src/conf/storage_conf.c | 11 ++++++++++-
>> 1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
>> index 4852dfb..c1bc242 100644
>> --- a/src/conf/storage_conf.c
>> +++ b/src/conf/storage_conf.c
>> @@ -2415,7 +2415,16 @@
virStoragePoolSourceMatchSingleHost(virStoragePoolSourcePtr poolsrc,
>> if (poolsrc->hosts[0].port != defsrc->hosts[0].port)
>> return false;
>>
>
> This function is called when parsing the configuration, which should not
> depend on host state.
>
> For example, if libvirt is started really early at boot time and the
> hostnames cannot be resolved by the DNS yet, they will pass the check
> but they will disappear on libvirtd restart.
Hmm... the downside of unreliable dependencies.
>
> The hostname->ip pairings are not stable either, so if we do this, I
> think it should be done on pool startup, not config parsing.
>
Right, but by the time we get to pool startup we'd be at the same point
as referenced above - if done early enough at boot time, then it's not
going to fail, but perhaps better than nothing.
Randomly failing to parse a config is worse than nothing.
I suppose at least moving to startup allows for better error paths since
currently errors can be overwritten or ignored.
>> - return STREQ(poolsrc->hosts[0].name, defsrc->hosts[0].name);
>> + if (STRNEQ(poolsrc->hosts[0].name, defsrc->hosts[0].name)) {
>> + /* Matching just a name isn't reliable as someone could provide
>> + * the name for one pool and the IP Address for another pool, so
>
> Resolving them is IMHO just as unreliable.
>
> Re: the original bug - is it possible to check that we have connected to
> a session with a different hostname than what we requested?
What does the connected session hostname have to do with the original
bug? The bug requests checking that an iSCSI pool doesn't use a "<host
name='<some IP Address>'..." for one pool:
<host name='10.66.6.12' port='3260'/>
and a resolved name for another pool on the same host:
<host name='test1' port='3260'/>
Where :
# cat /etc/hosts
10.66.6.12 test1
The bug pointed out iSCSI in particular, but since other pools use the
<source... <host name='%s'.../>... /> XML formatting it seemed
logical
to have them all use the same checks.
The bug requests that this should be forbidden when defining the pool
(which is NOTABUG), because we then allow starting both of them, but
stopping one of them also stops the other one, while libvirt still
thinks it's up (this is the real bug).
We definitely shouldn't resolve the hostnames when we parse the XML, the
XML parser/formatter should not depend on the host state.
Resolving the hostname on pool startup would at least remove the
roulette from XML parser, but still won't be foolproof -
multiple IP addresses can point to the same host.
More importantly, it won't solve the underlying issue:
We don't care what host we connect to.
As long as virStorageBackendISCSISession finds the <source><device path>
in the output of iscsiadm --mode session, the pool is marked as started.
While it might make sense to have the pool accessible via different
networks (or different address families), our iscsi backend is not ready
for that.
I don't know how to allow that, (without guessing what iscsiadm resolved
the hostname to), but marking the pool as started, if the session was
created by another pool is wrong.
>
> Or just disallow starting two pools with the same targetname, since they
> are supposed to be unique?
>
'targetname' as in ?
The <source><device path='xxxx'> attribute of the iscsi pool, which
we
feed to iscsiadm via --targetname.
Jan
12:17 p.m.
New subject: [libvirt] [PATCH 7/7] storage: Check for duplicate host for incoming pool def
...
>>> - return STREQ(poolsrc->hosts[0].name,
defsrc->hosts[0].name);
>>> + if (STRNEQ(poolsrc->hosts[0].name, defsrc->hosts[0].name)) {
>>> + /* Matching just a name isn't reliable as someone could provide
>>> + * the name for one pool and the IP Address for another pool, so
>>
>> Resolving them is IMHO just as unreliable.
>>
>> Re: the original bug - is it possible to check that we have connected to
>> a session with a different hostname than what we requested?
>
> What does the connected session hostname have to do with the original
> bug? The bug requests checking that an iSCSI pool doesn't use a "<host
> name='<some IP Address>'..." for one pool:
>
> <host name='10.66.6.12' port='3260'/>
>
> and a resolved name for another pool on the same host:
>
> <host name='test1' port='3260'/>
>
> Where :
>
> # cat /etc/hosts
> 10.66.6.12 test1
>
> The bug pointed out iSCSI in particular, but since other pools use the
> <source... <host name='%s'.../>... /> XML formatting it seemed
logical
> to have them all use the same checks.
>
The bug requests that this should be forbidden when defining the pool
(which is NOTABUG), because we then allow starting both of them, but
stopping one of them also stops the other one, while libvirt still
thinks it's up (this is the real bug).
Sure the part about causing an error at define time I see your point,
but extrapolating a bit at startup time it's still an issue to have two
pools pointing to the same source.
Just closing it NOTABUG because the wording isn't precise doesn't mean
that part of the bug still is valid and needs some sort of resolution.
Without letting two pools use the same network source host and device
target path, we'd never be faced with the shutdown one conundrum. We
disallow certain duplication at definition time, but network pools have
an extra problem of source host resolution that's not resolved (yet).
I can take my 'localhost' netfs pool, change a couple of parameters
(name to prefix with "dup-", targetpath to prefix with "dup-", and
host
name to be '127.0.0.1') and can start two pools up looking at the same
source.
The shutdown path is just "assuming" that the startup path disallowed
duplicate pools pointing at the same source paths, but I believe
allowing a pool to be started that's looking at the same target is still
a bug.
We definitely shouldn't resolve the hostnames when we parse the XML, the
XML parser/formatter should not depend on the host state.
Resolving the hostname on pool startup would at least remove the
roulette from XML parser, but still won't be foolproof -
multiple IP addresses can point to the same host.
More importantly, it won't solve the underlying issue:
We don't care what host we connect to.
As long as virStorageBackendISCSISession finds the <source><device path>
in the output of iscsiadm --mode session, the pool is marked as started.
While it might make sense to have the pool accessible via different
networks (or different address families), our iscsi backend is not ready
for that.
I don't know how to allow that, (without guessing what iscsiadm resolved
the hostname to), but marking the pool as started, if the session was
created by another pool is wrong.
Right - the iscsi backend has made extra assumptions based on the theory
that the source host checks ensured that the source host name wasn't
duplicated.
I know the NETFS pool doesn't suffer from the same destroy issue, I'm
not so sure about Gluster and Sheepdog as I don't have a configuration
of them handy.
>>
>> Or just disallow starting two pools with the same targetname, since they
>> are supposed to be unique?
>>
>
> 'targetname' as in ?
The <source><device path='xxxx'> attribute of the iscsi pool, which
we
feed to iscsiadm via --targetname.
Oh - OK - that check is done, but the results overridden if the
<source...> duplicate check fails. If we did have two different hosts,
then they could conceivably have the same target name/path. That could
be a way to configure some sort of hot standby or primary/secondary
where the device paths are the same, but the hosts change. For iSCSI
it'll create different iqn, portal results.
John
6:50 a.m.
New subject: [libvirt] [PATCH 7/7] storage: Check for duplicate host for incoming pool def
On Wed, Apr 22, 2015 at 01:17:20PM -0400, John Ferlan wrote:
...
>>>> - return STREQ(poolsrc->hosts[0].name, defsrc->hosts[0].name);
>>>> + if (STRNEQ(poolsrc->hosts[0].name, defsrc->hosts[0].name)) {
>>>> + /* Matching just a name isn't reliable as someone could
provide
>>>> + * the name for one pool and the IP Address for another pool,
so
>>>
>>> Resolving them is IMHO just as unreliable.
>>>
>>> Re: the original bug - is it possible to check that we have connected to
>>> a session with a different hostname than what we requested?
>>
>> What does the connected session hostname have to do with the original
>> bug? The bug requests checking that an iSCSI pool doesn't use a
"<host
>> name='<some IP Address>'..." for one pool:
>>
>> <host name='10.66.6.12' port='3260'/>
>>
>> and a resolved name for another pool on the same host:
>>
>> <host name='test1' port='3260'/>
>>
>> Where :
>>
>> # cat /etc/hosts
>> 10.66.6.12 test1
>>
>> The bug pointed out iSCSI in particular, but since other pools use the
>> <source... <host name='%s'.../>... /> XML formatting it
seemed logical
>> to have them all use the same checks.
>>
>
> The bug requests that this should be forbidden when defining the pool
> (which is NOTABUG), because we then allow starting both of them, but
> stopping one of them also stops the other one, while libvirt still
> thinks it's up (this is the real bug).
Sure the part about causing an error at define time I see your point,
but extrapolating a bit at startup time it's still an issue to have two
pools pointing to the same source.
Just closing it NOTABUG because the wording isn't precise doesn't mean
that part of the bug still is valid and needs some sort of resolution.
I don't think we should guard everything and put pillows on every sharp
edge. Doing a STREQ on the provided source paths provides some
convenience to the user, with little effort. Resolving every hostname is
much more effort and not that foolproof.
Yes, if we leave the check out, you will be able to start two pools
pointing to the same host. Changes done to one pool won't be reflected in the
other. We also allow to two domains pointing to the same storage.
Without letting two pools use the same network source host and
device
target path, we'd never be faced with the shutdown one conundrum. We
disallow certain duplication at definition time, but network pools have
an extra problem of source host resolution that's not resolved (yet).
I can take my 'localhost' netfs pool, change a couple of parameters
(name to prefix with "dup-", targetpath to prefix with "dup-", and
host
name to be '127.0.0.1') and can start two pools up looking at the same
source.
The shutdown path is just "assuming" that the startup path disallowed
duplicate pools pointing at the same source paths, but I believe
allowing a pool to be started that's looking at the same target is still
a bug.
For netfs, I don't see the problem - we create a separate mount, and
unmount it separately. (We don't actually check if it's the requested
host that's mounted there, but at least it doesn't block shutdown).
>
> We definitely shouldn't resolve the hostnames when we parse the XML, the
> XML parser/formatter should not depend on the host state.
>
> Resolving the hostname on pool startup would at least remove the
> roulette from XML parser, but still won't be foolproof -
> multiple IP addresses can point to the same host.
> More importantly, it won't solve the underlying issue:
>
> We don't care what host we connect to.
>
> As long as virStorageBackendISCSISession finds the <source><device
path>
> in the output of iscsiadm --mode session, the pool is marked as started.
>
> While it might make sense to have the pool accessible via different
> networks (or different address families), our iscsi backend is not ready
> for that.
>
> I don't know how to allow that, (without guessing what iscsiadm resolved
> the hostname to), but marking the pool as started, if the session was
> created by another pool is wrong.
>
Right - the iscsi backend has made extra assumptions based on the theory
that the source host checks ensured that the source host name wasn't
duplicated.
No, it made the assumptions purely on the source device path. Adding a
source host check won't solve the issue on the shutdown path.
We should either disallow duplicate device paths, or handle them
correctly. Doing so in the XML parser feels wrong, but we already
check for duplicate paths there (I think the only reason it is done
on parsing is because we automatically mark any pools that are already
mounted as started).
I know the NETFS pool doesn't suffer from the same destroy issue,
I'm
not so sure about Gluster and Sheepdog as I don't have a configuration
of them handy.
>
>>>
>>> Or just disallow starting two pools with the same targetname, since they
>>> are supposed to be unique?
>>>
>>
>> 'targetname' as in ?
>
> The <source><device path='xxxx'> attribute of the iscsi pool,
which we
> feed to iscsiadm via --targetname.
>
Oh - OK - that check is done, but the results overridden if the
<source...> duplicate check fails.
I don't see the check being done anywhere.
If we did have two different hosts,
then they could conceivably have the same target name/path. That could
be a way to configure some sort of hot standby or primary/secondary
where the device paths are the same, but the hosts change. For iSCSI
it'll create different iqn, portal results.
The iscsi backend doesn't care about iqn an portal results, if it sees a
session with a matching device path.
Jan
John
11:37 a.m.
New subject: [libvirt] [PATCH 7/7] storage: Check for duplicate host for incoming pool def
On 04/23/2015 07:50 AM, Ján Tomko wrote:
On Wed, Apr 22, 2015 at 01:17:20PM -0400, John Ferlan wrote:
...
Having the dialog in your other series caused me to remember there was
still a question here.
>>
>>>>
>>>> Or just disallow starting two pools with the same targetname, since they
>>>> are supposed to be unique?
>>>>
>>>
>>> 'targetname' as in ?
>>
>> The <source><device path='xxxx'> attribute of the iscsi pool,
which we
>> feed to iscsiadm via --targetname.
>>
>
> Oh - OK - that check is done, but the results overridden if the
> <source...> duplicate check fails.
I don't see the check being done anywhere.
virStoragePoolSourceFindDuplicate for VIR_STORAGE_POOL_ISCSI calls
virStoragePoolSourceFindDuplicateDevices which does the dual pool search
through source.devices[i].path (or the IQN for the device).
If a duplicate is found, then it would check if the source hostname's
match. The formatstorage page describes "Will be used in combination
with a directory or device element." with regards to unique
identification. Since port is optional, even that check is sketchy since
one could have the same name, but not provide the port number in the
incoming definition and thus have a duplicate.
I suppose allowing two different 'iscsid' servers to advertise the same
"name" isn't necessarily an issue. It could allow for someone to set up
some sort of hot standby or backup (or who knows what) on separate
servers without needing to manage the IQN mapping between the two.
In the long run, the path in a vol-list is a combination of
/dev/disk-by-path (or <target... <path>...>), the host name/addr, and
the IQN such as:
/dev/disk/by-path/ip-192.168.122.1:3260-iscsi-iqn.2013-12.com.example:iscsi-chap-netpool-lun-1
and this links to the block device on the host (eg, ../../dev/sdb).
If there was a second pool started using the same "host" as the first
pool, but just by a different name, it too would use the same block
device, so now there would be two pools using the same device. Since
using the same device isn't allowed for other pools, the iSCSI pool
should also block usage. And yes, a completely separate host using the
same IQN would also use the same block device (but that's a different
bug IMO).
Perhaps the ultimate root cause is as you pointed out along the way that
virStorageBackendISCSISession just looks for (and assumes) the
'targetname' is unique for the host (assumes that the hostname checks
already rooted out differences). But that shows the second bug with
this code is that we could have the two hosts with the same IQN on each,
but our search will only ever find the "first" one (the regex in
virISCSIExtractSession doesn't compare hostnames).
So while even my .last of this and my v2 series doesn't resolve the
second issue, I believe they do at least inhibit the same host by a
different name issue.
If the v2 of the series was accepted, then the "next step" would be to
add/extract that hostname:port from the iscsiadm command and determine
whether it "matched" the expected one so that we could find that
theoretical second/different host using the same IQN as the first.
John
4:59 a.m.
New subject: [libvirt] [PATCH 7/7] storage: Check for duplicate host for incoming pool def
On Wed, Apr 29, 2015 at 12:37:38PM -0400, John Ferlan wrote:
On 04/23/2015 07:50 AM, Ján Tomko wrote:
> On Wed, Apr 22, 2015 at 01:17:20PM -0400, John Ferlan wrote:
...
Having the dialog in your other series caused me to remember there was
still a question here.
>>>
>>>>>
>>>>> Or just disallow starting two pools with the same targetname, since
they
>>>>> are supposed to be unique?
>>>>>
>>>>
>>>> 'targetname' as in ?
>>>
>>> The <source><device path='xxxx'> attribute of the iscsi
pool, which we
>>> feed to iscsiadm via --targetname.
>>>
>>
>> Oh - OK - that check is done, but the results overridden if the
>> <source...> duplicate check fails.
>
> I don't see the check being done anywhere.
>
virStoragePoolSourceFindDuplicate for VIR_STORAGE_POOL_ISCSI calls
virStoragePoolSourceFindDuplicateDevices which does the dual pool search
through source.devices[i].path (or the IQN for the device).
If a duplicate is found, then it would check if the source hostname's
match. The formatstorage page describes "Will be used in combination
with a directory or device element." with regards to unique
identification.
That is the generic description shared with all the pools. Maybe the
docs need some clarification?
Since port is optional, even that check is sketchy since
one could have the same name, but not provide the port number in the
incoming definition and thus have a duplicate.
I suppose allowing two different 'iscsid' servers to advertise the same
"name" isn't necessarily an issue. It could allow for someone to set up
some sort of hot standby or backup (or who knows what) on separate
servers without needing to manage the IQN mapping between the two.
Even though it is possible to configure two servers with the same IQN,
I don't see why we should support starting pools with both of them at
at the same time.
In the long run, the path in a vol-list is a combination of
/dev/disk-by-path (or <target... <path>...>), the host name/addr, and
the IQN such as:
/dev/disk/by-path/ip-192.168.122.1:3260-iscsi-iqn.2013-12.com.example:iscsi-chap-netpool-lun-1
and this links to the block device on the host (eg, ../../dev/sdb).
If there was a second pool started using the same "host" as the first
pool, but just by a different name, it too would use the same block
device, so now there would be two pools using the same device. Since
using the same device isn't allowed for other pools, the iSCSI pool
should also block usage.
So we have bug A - starting two pools from the same host with the same IQN
And yes, a completely separate host using the
same IQN would also use the same block device (but that's a different
bug IMO).
and bug B - starting two pools from different hosts with the same IQN
Since we can't reliably tell it the hosts are different or the same,
and I doubt the usefulness of two different hosts using the same IQN,
why not just reduce this to one bug and reject duplicate IQNs regardless
of the host?
Perhaps the ultimate root cause is as you pointed out along the way
that
virStorageBackendISCSISession just looks for (and assumes) the
'targetname' is unique for the host (assumes that the hostname checks
already rooted out differences). But that shows the second bug with
this code is that we could have the two hosts with the same IQN on each,
but our search will only ever find the "first" one (the regex in
virISCSIExtractSession doesn't compare hostnames).
The iscsiadm output only contains IP addresses for me.
So while even my .last of this and my v2 series doesn't resolve the
second issue, I believe they do at least inhibit the same host by a
different name issue.
If the v2 of the series was accepted, then the "next step" would be to
add/extract that hostname:port from the iscsiadm command and determine
whether it "matched" the expected one so that we could find that
theoretical second/different host using the same IQN as the first.
I don't think doing that much work just to work around misconfigured
systems is worth it.
Jan
9:16 a.m.
New subject: [libvirt] [PATCH 7/7] storage: Check for duplicate host for incoming pool def
On 04/30/2015 05:59 AM, Ján Tomko wrote:
On Wed, Apr 29, 2015 at 12:37:38PM -0400, John Ferlan wrote:
>
>
> On 04/23/2015 07:50 AM, Ján Tomko wrote:
>> On Wed, Apr 22, 2015 at 01:17:20PM -0400, John Ferlan wrote:
> ...
>
> Having the dialog in your other series caused me to remember there was
> still a question here.
>
>>>>
>>>>>>
>>>>>> Or just disallow starting two pools with the same targetname,
since they
>>>>>> are supposed to be unique?
>>>>>>
>>>>>
>>>>> 'targetname' as in ?
>>>>
>>>> The <source><device path='xxxx'> attribute of the
iscsi pool, which we
>>>> feed to iscsiadm via --targetname.
>>>>
>>>
>>> Oh - OK - that check is done, but the results overridden if the
>>> <source...> duplicate check fails.
>>
>> I don't see the check being done anywhere.
>>
>
> virStoragePoolSourceFindDuplicate for VIR_STORAGE_POOL_ISCSI calls
> virStoragePoolSourceFindDuplicateDevices which does the dual pool search
> through source.devices[i].path (or the IQN for the device).
>
> If a duplicate is found, then it would check if the source hostname's
> match. The formatstorage page describes "Will be used in combination
> with a directory or device element." with regards to unique
> identification.
That is the generic description shared with all the pools. Maybe the
docs need some clarification?
So you feel it's OK or better to document and restrict something that
could essentially work given some code? IOW: For this one pool type it
is better to restrict based solely on the IQN - that's a preferable
solution? Because it's not worth doing that much work just to work
around misconfigured systems or a perception of misconfiguration?
> Since port is optional, even that check is sketchy since
> one could have the same name, but not provide the port number in the
> incoming definition and thus have a duplicate.
>
> I suppose allowing two different 'iscsid' servers to advertise the same
> "name" isn't necessarily an issue. It could allow for someone to set
up
> some sort of hot standby or backup (or who knows what) on separate
> servers without needing to manage the IQN mapping between the two.
>
Even though it is possible to configure two servers with the same IQN,
I don't see why we should support starting pools with both of them at
at the same time.
Because we have already supported it and technically it can work if
you've done your configuration correctly and (more or less) know what
you're doing.
> In the long run, the path in a vol-list is a combination of
> /dev/disk-by-path (or <target... <path>...>), the host name/addr, and
> the IQN such as:
>
>
/dev/disk/by-path/ip-192.168.122.1:3260-iscsi-iqn.2013-12.com.example:iscsi-chap-netpool-lun-1
>
> and this links to the block device on the host (eg, ../../dev/sdb).
>
> If there was a second pool started using the same "host" as the first
> pool, but just by a different name, it too would use the same block
> device, so now there would be two pools using the same device. Since
> using the same device isn't allowed for other pools, the iSCSI pool
> should also block usage.
So we have bug A - starting two pools from the same host with the same IQN
> And yes, a completely separate host using the
> same IQN would also use the same block device (but that's a different
> bug IMO).
>
and bug B - starting two pools from different hosts with the same IQN
Since we can't reliably tell it the hosts are different or the same,
and I doubt the usefulness of two different hosts using the same IQN,
why not just reduce this to one bug and reject duplicate IQNs regardless
of the host?
IMO, using address name resolution while not a "perfect solution" is
better than deciding to disallow something that someone may have a
reason to configure in such a manner. If the name resolution causes an
issue due to unreliable DNS or some other DNS factor beyond the scope of
libvirt, then the host configuration has far greater issues. I could
just as easily claim I doubt someone would have such an unreliable DNS
configuration, but I'm sure I'd be wrong too! Still having an
unreliable DNS is allowed.
> Perhaps the ultimate root cause is as you pointed out along the
way that
> virStorageBackendISCSISession just looks for (and assumes) the
> 'targetname' is unique for the host (assumes that the hostname checks
> already rooted out differences). But that shows the second bug with
> this code is that we could have the two hosts with the same IQN on each,
> but our search will only ever find the "first" one (the regex in
> virISCSIExtractSession doesn't compare hostnames).
The iscsiadm output only contains IP addresses for me.
From my quick read of the source code, the open-iscsi code uses
getaddrinfo for output. And the session matching code uses a two level
loop (usr/iscsi_util.c - iscsi_addr_match()) which compares a stored
address against the incoming/proposed session address (uses ai_addrlen
and ai_addr comparisons) after the getaddrinfo.
So essentially not much different than what I proposed. The open-iscsi
code though is claiming the two are the same, so it won't create a new
session which is why we get into the situation we get into.
>
> So while even my .last of this and my v2 series doesn't resolve the
> second issue, I believe they do at least inhibit the same host by a
> different name issue.
>
> If the v2 of the series was accepted, then the "next step" would be to
> add/extract that hostname:port from the iscsiadm command and determine
> whether it "matched" the expected one so that we could find that
> theoretical second/different host using the same IQN as the first.
I don't think doing that much work just to work around misconfigured
systems is worth it.
There is a saying "Imitation is the sincerest form of flattery"... I
wonder how many people just cut-n-paste what's in the docs to get things
to work? In this case, it may be considered babysitting those that
aren't as proficient, but it may also help in the case where someone
doesn't realize their mistake. While we may find the practice of copying
a configuration that works less than desirable, it can happen. While we
don't think someone would configure one pool using the IP Address and
another pool using a name, it can happen.
I think restricting on the same IQN takes a big hammer approach, but if
that's what is desired because it's felt that it's either too much code
to add or because DNS is misconfigured, misbehaving, or not reliable
enough, then while I technically disagree, I suppose since we can make
up the rules to play by.
BTW: Beyond this bz (1171984), there is an iSCSI ipv4 vs. ipv6 host name
configuration issue described in bz1188463 and bz1207929 which describes
a usage with gluster volume lookups where a failure is declared
seemingly because the pool is defined with the IP address while the
vol-name lookup was done using a name. The changes in v2 could easily be
"reused" in order to ascertain that the name and number match. Beyond
that it'd be up the gluster code to do it's own resolution in order to
find the target.
John
8:44 a.m.
New subject: [libvirt] [PATCH 7/7] storage: Check for duplicate host for incoming pool def
On Thu, Apr 30, 2015 at 10:16:55AM -0400, John Ferlan wrote:
On 04/30/2015 05:59 AM, Ján Tomko wrote:
> On Wed, Apr 29, 2015 at 12:37:38PM -0400, John Ferlan wrote:
>> virStoragePoolSourceFindDuplicate for VIR_STORAGE_POOL_ISCSI calls
>> virStoragePoolSourceFindDuplicateDevices which does the dual pool search
>> through source.devices[i].path (or the IQN for the device).
>>
>> If a duplicate is found, then it would check if the source hostname's
>> match. The formatstorage page describes "Will be used in combination
>> with a directory or device element." with regards to unique
>> identification.
>
> That is the generic description shared with all the pools. Maybe the
> docs need some clarification?
>
So you feel it's OK or better to document and restrict something that
could essentially work given some code? IOW: For this one pool type it
is better to restrict based solely on the IQN - that's a preferable
solution? Because it's not worth doing that much work just to work
around misconfigured systems or a perception of misconfiguration?
d/perception of / ;)
Yes.
>> Since port is optional, even that check is sketchy since
>> one could have the same name, but not provide the port number in the
>> incoming definition and thus have a duplicate.
>>
>> I suppose allowing two different 'iscsid' servers to advertise the same
>> "name" isn't necessarily an issue. It could allow for someone to
set up
>> some sort of hot standby or backup (or who knows what) on separate
>> servers without needing to manage the IQN mapping between the two.
>>
>
> Even though it is possible to configure two servers with the same IQN,
> I don't see why we should support starting pools with both of them at
> at the same time.
>
Because we have already supported it and technically it can work if
you've done your configuration correctly and (more or less) know what
you're doing.
But we haven't supported it - starting the second pool will
short-circuit because we already see a session with that IQN there.
>> In the long run, the path in a vol-list is a combination of
>> /dev/disk-by-path (or <target... <path>...>), the host name/addr,
and
>> the IQN such as:
>>
>>
/dev/disk/by-path/ip-192.168.122.1:3260-iscsi-iqn.2013-12.com.example:iscsi-chap-netpool-lun-1
>>
>> and this links to the block device on the host (eg, ../../dev/sdb).
>>
>> If there was a second pool started using the same "host" as the first
>> pool, but just by a different name, it too would use the same block
>> device, so now there would be two pools using the same device. Since
>> using the same device isn't allowed for other pools, the iSCSI pool
>> should also block usage.
>
> So we have bug A - starting two pools from the same host with the same IQN
>
>> And yes, a completely separate host using the
>> same IQN would also use the same block device (but that's a different
>> bug IMO).
>>
>
> and bug B - starting two pools from different hosts with the same IQN
>
> Since we can't reliably tell it the hosts are different or the same,
> and I doubt the usefulness of two different hosts using the same IQN,
> why not just reduce this to one bug and reject duplicate IQNs regardless
> of the host?
>
IMO, using address name resolution while not a "perfect solution" is
better than deciding to disallow something that someone may have a
reason to configure in such a manner. If the name resolution causes an
issue due to unreliable DNS or some other DNS factor beyond the scope of
libvirt, then the host configuration has far greater issues. I could
just as easily claim I doubt someone would have such an unreliable DNS
configuration, but I'm sure I'd be wrong too! Still having an
unreliable DNS is allowed.
The difference would be in the amount of code added to libvirt. :)
BTW: Beyond this bz (1171984), there is an iSCSI ipv4 vs. ipv6 host name
configuration issue described in bz1188463 and bz1207929 which describes
a usage with gluster volume lookups where a failure is declared
seemingly because the pool is defined with the IP address while the
vol-name lookup was done using a name. The changes in v2 could easily be
"reused" in order to ascertain that the name and number match. Beyond
that it'd be up the gluster code to do it's own resolution in order to
find the target.
The lookup code in v2 can be used by gluster regardless of its usage by
iSCSI pool (where checking the IQN should be enough) or NFS (where I'm
not sure we want to check the hostname at all - historically we've
allowed starting a NFS pool as long as there was something mounted to
the target path).
John
Jan
2:57 p.m.
New subject: [libvirt] [PATCH 7/7] storage: Check for duplicate host for incoming pool def
On 05/04/2015 09:44 AM, Ján Tomko wrote:
On Thu, Apr 30, 2015 at 10:16:55AM -0400, John Ferlan wrote:
> On 04/30/2015 05:59 AM, Ján Tomko wrote:
>> On Wed, Apr 29, 2015 at 12:37:38PM -0400, John Ferlan wrote:
>>> virStoragePoolSourceFindDuplicate for VIR_STORAGE_POOL_ISCSI calls
>>> virStoragePoolSourceFindDuplicateDevices which does the dual pool search
>>> through source.devices[i].path (or the IQN for the device).
>>>
>>> If a duplicate is found, then it would check if the source hostname's
>>> match. The formatstorage page describes "Will be used in combination
>>> with a directory or device element." with regards to unique
>>> identification.
>>
>> That is the generic description shared with all the pools. Maybe the
>> docs need some clarification?
>>
>
> So you feel it's OK or better to document and restrict something that
> could essentially work given some code? IOW: For this one pool type it
> is better to restrict based solely on the IQN - that's a preferable
> solution? Because it's not worth doing that much work just to work
> around misconfigured systems or a perception of misconfiguration?
>
d/perception of / ;)
Yes.
Wasn't too much work for iscsid which is able to "use" an existing
session if it determines the IP Addresses are the same. Technically it's
not a misconfiguration, it seems just an artificial limitation due to an
inability to trust DNS name resolutions.
>>> Since port is optional, even that check is sketchy since
>>> one could have the same name, but not provide the port number in the
>>> incoming definition and thus have a duplicate.
>>>
>>> I suppose allowing two different 'iscsid' servers to advertise the
same
>>> "name" isn't necessarily an issue. It could allow for someone
to set up
>>> some sort of hot standby or backup (or who knows what) on separate
>>> servers without needing to manage the IQN mapping between the two.
>>>
>>
>> Even though it is possible to configure two servers with the same IQN,
>> I don't see why we should support starting pools with both of them at
>> at the same time.
>>
>
> Because we have already supported it and technically it can work if
> you've done your configuration correctly and (more or less) know what
> you're doing.
>
But we haven't supported it - starting the second pool will
short-circuit because we already see a session with that IQN there.
Whether "we" saw it or not, iscsid will "match" if it determines that
the host:port portion is a duplicate.
>>> In the long run, the path in a vol-list is a combination
of
>>> /dev/disk-by-path (or <target... <path>...>), the host name/addr,
and
>>> the IQN such as:
>>>
>>>
/dev/disk/by-path/ip-192.168.122.1:3260-iscsi-iqn.2013-12.com.example:iscsi-chap-netpool-lun-1
>>>
>>> and this links to the block device on the host (eg, ../../dev/sdb).
>>>
>>> If there was a second pool started using the same "host" as the
first
>>> pool, but just by a different name, it too would use the same block
>>> device, so now there would be two pools using the same device. Since
>>> using the same device isn't allowed for other pools, the iSCSI pool
>>> should also block usage.
>>
>> So we have bug A - starting two pools from the same host with the same IQN
>>
>>> And yes, a completely separate host using the
>>> same IQN would also use the same block device (but that's a different
>>> bug IMO).
>>>
>>
>> and bug B - starting two pools from different hosts with the same IQN
>>
>> Since we can't reliably tell it the hosts are different or the same,
>> and I doubt the usefulness of two different hosts using the same IQN,
>> why not just reduce this to one bug and reject duplicate IQNs regardless
>> of the host?
>>
>
>
> IMO, using address name resolution while not a "perfect solution" is
> better than deciding to disallow something that someone may have a
> reason to configure in such a manner. If the name resolution causes an
> issue due to unreliable DNS or some other DNS factor beyond the scope of
> libvirt, then the host configuration has far greater issues. I could
> just as easily claim I doubt someone would have such an unreliable DNS
> configuration, but I'm sure I'd be wrong too! Still having an
> unreliable DNS is allowed.
>
The difference would be in the amount of code added to libvirt. :)
Not sure that should be a reason to not accept something. The first set
of patches associated with virIsValidHostname were an "afterthought"
after adding the name resolution checking code. There were added
because I thought it might be a good thing to check and/or notify that
the name to be used wasn't valid. Allowing netfs, gluster, iscsi, and
sheepdog to go through their startups only to fail is still an option. I
guess it all depends on what you consider too much
>
> BTW: Beyond this bz (1171984), there is an iSCSI ipv4 vs. ipv6 host name
> configuration issue described in bz1188463 and bz1207929 which describes
> a usage with gluster volume lookups where a failure is declared
> seemingly because the pool is defined with the IP address while the
> vol-name lookup was done using a name. The changes in v2 could easily be
> "reused" in order to ascertain that the name and number match. Beyond
> that it'd be up the gluster code to do it's own resolution in order to
> find the target.
The lookup code in v2 can be used by gluster regardless of its usage by
iSCSI pool (where checking the IQN should be enough) or NFS (where I'm
not sure we want to check the hostname at all - historically we've
allowed starting a NFS pool as long as there was something mounted to
the target path).
So it seems at least patch 6 is useful and that's the bulk of the code.
Patch 7 just uses that. Whether patches 1-5 are dropped due to code
bloat is no big deal since the server startup will error anyway (and in
the case of gluster leak memory according to Peter's description).
John
3492
days inactive
3506
days old
33 comments
3 participants
participants (3)
-
John Ferlan -
Ján Tomko -
Peter Krempa