[libvirt] [PATCH 0/7] Addition host name check for network storage pools
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
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@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
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@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
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@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
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@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
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
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
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
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@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
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@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
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@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
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@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
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@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
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@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
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@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
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@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)
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@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
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@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
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@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
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@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
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@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
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@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
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@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
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@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
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@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
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@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
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@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
...
- 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
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
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
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
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
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
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
participants (3)
-
John Ferlan -
Ján Tomko -
Peter Krempa