Re: [libvirt] [RFC 0/7] Live Migration with Pass-through Devices proposal
On 04/17/2015 04:53 AM, Chen Fan wrote: backgrond: Live migration is one of the most important features of virtualization technology. With regard to recent virtualization techniques, performance of network I/O is critical. Current network I/O virtualization (e.g. Para-virtualized I/O, VMDq) has a significant performance gap with native network I/O. Pass-through network devices have near native performance, however, they have thus far prevented live migration. No existing methods solve the problem of live migration with pass-through devices perfectly. There was an idea to solve the problem in website: https://www.kernel.org/doc/ols/2008/ols2008v2-pages-261-267.pdf Please refer to above document for detailed information. This functionality has been on my mind/bug list for a long time, but I haven't been able to pursue it much. See this BZ, along with the original patches submitted by Shradha Shah from SolarFlare: https://bugzilla.redhat.com/show_bug.cgi?id=896716 (I was a bit optimistic in my initial review of the patches - there are actually a lot of issues that weren't handled by those patches.) So I think this problem maybe could be solved by using the combination of existing technology. and the following steps are we considering to implement: - before boot VM, we anticipate to specify two NICs for creating bonding device (one plugged and one virtual NIC) in XML. here we can specify the NIC's mac addresses in XML, which could facilitate qemu-guest-agent to find the network interfaces in guest. An interesting idea, but I think that is a 2nd level enhancement, not necessary initially (and maybe not ever, due to the high possibility of it being extremely difficult to get right in 100% of the cases). - when qemu-guest-agent startup in guest it would send a notification to libvirt, then libvirt will call the previous registered initialize callbacks. so through the callback functions, we can create the bonding device according to the XML configuration. and here we use netcf tool which can facilitate to create bonding device easily. This isn't quite making sense - the bond will be on the guest, which may not have netcf installed. Anyway, I think it should be up to the guest's own system network config to have the bond already setup. If you try to impose it from outside that infrastructure, you run too much risk of running afoul of something on the guest (e.g. NetworkManager) - during migration, unplug the passthroughed NIC. then do native migration. Correct. This is the most important part. But not just unplugging it, you also need to wait until the unplug operation completes (it is asynchronous). (After this point, the emulated NIC that is part of the bond would get all of the traffic). - on destination side, check whether need to hotplug new NIC according to specified XML. usually, we use migrate --xml command option to specify the destination host NIC mac address to hotplug a new NIC, because source side passthrough NIC mac address is different, then hotplug the deivce according to the destination XML configuration. Why does the MAC address need to be different? Are you suggesting doing this with passed-through non-SRIOV NICs? An SRIOV virtual function gets its MAC address from the libvirt config, so it's very simple to use the same MAC address across the migration. Any network card that would be able to do this on any sort of useful scale will be SRIOV-capable (or should be replaced with one that is - some of them are not that expensive). TODO: 1. when hot add a new NIC in destination side after migration finished, the NIC device need to re-enslave on bonding device in guest. otherwise, it is offline. maybe we should consider bonding driver to support add interfaces dynamically. I never looked at the details of how SolarFlare's code handled the guest side (they have/had their own patchset they maintained for some older version of libvirt which integrated with some sort of enhanced bonding driver on the guests). I assumed the bond driver could handle this already, but have to say I never investigated. This is an example on how this might work, so I want to hear some voices about this scenario. Thanks, Chen Chen Fan (7): qemu-agent: add agent init callback when detecting guest setup qemu: add guest init event callback to do the initialize work for guest hostdev: add a 'bond' type element in hostdev element Putting this into hostdev is the wrong approach, for two reasons: 1) it doesn't account for the device to be used being in a different address on the source and destination hosts, 2) the interface element already has much of the config you need, and an interface type supporting hostdev passthrough. It has been possible to do passthrough of an SRIOV VF via interface type='hostdev' for a long time now and, even better, via an interface
[libvirt] [PATCH 3/7] netfs: Check for validity of pool source hostname
Before attempting to mount the netfs pool, ensure the source pool
hostname can be resolved to something this host knows.
Signed-off-by: John Ferlan jfer...@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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 7/7] storage: Check for duplicate host for incoming pool def
https://bugzilla.redhat.com/show_bug.cgi?id=1171984
Use the new virIsSameHostnameInfo API to determine whether the proposed
storage pool definition matches the existing storage pool definition
Signed-off-by: John Ferlan jfer...@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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 4/7] gluster: Check for validity of pool source hostname
Prior to attempting to open the gluster connection, let's make sure we can resolve the source pool hostname. Signed-off-by: John Ferlan jfer...@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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/7] iscsi: Check for validity of pool source hostname
Ensure that the pool that's being started has a source pool hostname
that can be resolved before trying to start an iSCSI session.
Signed-off-by: John Ferlan jfer...@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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/7] virutil: Introduce virIsValidHostname
Similar to virGetHostname, but this time taking a parameter which is a
hostname or ipaddress from a source ... host name ='%s'.../... /
XML property and validating that the name can be resolved.
Return true or false depending on whether we can ascertain the hostname
address from calls to 'getnameinfo' and 'getaddrinfo'. Subsequent patches
will be validating a proposed pool hostname definition against existing
pool hostnames to ensure they are not the same hostname (and thus having
two pools looking at the same data)
Signed-off-by: John Ferlan jfer...@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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 6/7] util: Introduce virIsSameHostnameInfo
In order to assist with existing pool source hostname validation against
an incoming pool source definition, we need a way to validate that the
resolved hostname provided for the pool doesn't match some existing
pool; otherwise, we'd have a scenario where two storage pools could be
looking at the same data.
Search through all the existing pools resolved names against the proposed
definitions resolved hostnames - if any match, we return true; otherwise,
false is returned
Signed-off-by: John Ferlan jfer...@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)
+
[libvirt] [PATCH 5/7] sheepdog: Check for validity of pool source hostname
If there is a pool source hostname provided, let's make sure the name
can be resolved before trying to connect to it via a virCommand sequence.
Signed-off-by: John Ferlan jfer...@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);
[libvirt] [PATCH v4 3/4] scsi: Change return values for virStorageBackendSCSIFindLUs
Rather than passing/returning a pointer to a boolean to indicate that
perhaps we should try again - adjust the return of the call to return
the count of LU's found during processing, then let the caller decide
what to do with that value.
Signed-off-by: John Ferlan jfer...@redhat.com
---
src/storage/storage_backend_scsi.c | 34 ++
1 file changed, 14 insertions(+), 20 deletions(-)
diff --git a/src/storage/storage_backend_scsi.c
b/src/storage/storage_backend_scsi.c
index ae3cd9a..b98311c 100644
--- a/src/storage/storage_backend_scsi.c
+++ b/src/storage/storage_backend_scsi.c
@@ -430,10 +430,9 @@ processLU(virStoragePoolObjPtr pool,
}
-static int
-virStorageBackendSCSIFindLUsInternal(virStoragePoolObjPtr pool,
- uint32_t scanhost,
- bool *found)
+int
+virStorageBackendSCSIFindLUs(virStoragePoolObjPtr pool,
+ uint32_t scanhost)
{
int retval = 0;
uint32_t bus, target, lun;
@@ -441,6 +440,7 @@ virStorageBackendSCSIFindLUsInternal(virStoragePoolObjPtr
pool,
DIR *devicedir = NULL;
struct dirent *lun_dirent = NULL;
char devicepattern[64];
+int found = 0;
VIR_DEBUG(Discovering LUs on host %u, scanhost);
@@ -456,7 +456,6 @@ virStorageBackendSCSIFindLUsInternal(virStoragePoolObjPtr
pool,
snprintf(devicepattern, sizeof(devicepattern), %u:%%u:%%u:%%u\n,
scanhost);
-*found = false;
while ((retval = virDirRead(devicedir, lun_dirent, device_path)) 0) {
if (sscanf(lun_dirent-d_name, devicepattern,
bus, target, lun) != 3) {
@@ -466,25 +465,20 @@ virStorageBackendSCSIFindLUsInternal(virStoragePoolObjPtr
pool,
VIR_DEBUG(Found possible LU '%s', lun_dirent-d_name);
if (processLU(pool, scanhost, bus, target, lun) == 0)
-*found = true;
+found++;
}
-if (!*found)
-VIR_DEBUG(No LU found for pool %s, pool-def-name);
-
closedir(devicedir);
-return retval;
-}
+if (retval 0)
+return -1;
-int
-virStorageBackendSCSIFindLUs(virStoragePoolObjPtr pool,
- uint32_t scanhost)
-{
-bool found; /* This path doesn't care whether found or not */
-return virStorageBackendSCSIFindLUsInternal(pool, scanhost, found);
+VIR_DEBUG(Found %d LUs for pool %s, found, pool-def-name);
+
+return found;
}
+
static int
virStorageBackendSCSITriggerRescan(uint32_t host)
{
@@ -571,7 +565,7 @@ virStoragePoolFCRefreshThread(void *opaque)
const char *name = cbdata-name;
virStoragePoolObjPtr pool = cbdata-pool;
unsigned int host;
-bool found = false;
+int found;
int tries = 2;
do {
@@ -587,7 +581,7 @@ virStoragePoolFCRefreshThread(void *opaque)
virGetSCSIHostNumber(name, host) == 0
virStorageBackendSCSITriggerRescan(host) == 0) {
virStoragePoolObjClearVols(pool);
-virStorageBackendSCSIFindLUsInternal(pool, host, found);
+found = virStorageBackendSCSIFindLUs(pool, host);
}
virStoragePoolObjUnlock(pool);
} while (!found --tries);
@@ -910,7 +904,7 @@ virStorageBackendSCSIRefreshPool(virConnectPtr conn
ATTRIBUTE_UNUSED,
if (virStorageBackendSCSITriggerRescan(host) 0)
goto out;
-virStorageBackendSCSIFindLUs(pool, host);
+ignore_value(virStorageBackendSCSIFindLUs(pool, host));
ret = 0;
out:
--
2.1.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v4 1/4] storage: Split out the valid path check
For virStorageBackendStablePath, in order to make decisions in other code
split out the checks regarding whether the pool's target is empty, using /dev,
using /dev/, or doesn't start with /dev
Signed-off-by: John Ferlan jfer...@redhat.com
---
src/storage/storage_backend.c | 26 +-
src/storage/storage_backend.h | 1 +
2 files changed, 14 insertions(+), 13 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index 0435983..b07e0d9 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -1674,6 +1674,17 @@
virStorageBackendUpdateVolTargetInfoFD(virStorageSourcePtr target,
return 0;
}
+bool
+virStorageBackendPoolPathIsStable(const char *path)
+{
+if (path == NULL || STREQ(path, /dev) || STREQ(path, /dev/))
+return false;
+
+if (!STRPREFIX(path, /dev))
+return false;
+
+return true;
+}
/*
* Given a volume path directly in /dev/XXX, iterate over the
@@ -1703,20 +1714,9 @@ virStorageBackendStablePath(virStoragePoolObjPtr pool,
int retry = 0;
int direrr;
-/* Short circuit if pool has no target, or if its /dev */
-if (pool-def-target.path == NULL ||
-STREQ(pool-def-target.path, /dev) ||
-STREQ(pool-def-target.path, /dev/))
-goto ret_strdup;
-
-/* Skip whole thing for a pool which isn't in /dev
- * so we don't mess filesystem/dir based pools
- */
-if (!STRPREFIX(pool-def-target.path, /dev))
-goto ret_strdup;
-
/* Logical pools are under /dev but already have stable paths */
-if (pool-def-type == VIR_STORAGE_POOL_LOGICAL)
+if (pool-def-type == VIR_STORAGE_POOL_LOGICAL ||
+!virStorageBackendPoolPathIsStable(pool-def-target.path))
goto ret_strdup;
/* We loop here because /dev/disk/by-{id,path} may not have existed
diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h
index bb1e8d8..85a8a4b 100644
--- a/src/storage/storage_backend.h
+++ b/src/storage/storage_backend.h
@@ -187,6 +187,7 @@ int
virStorageBackendUpdateVolTargetInfoFD(virStorageSourcePtr target,
int fd,
struct stat *sb);
+bool virStorageBackendPoolPathIsStable(const char *path);
char *virStorageBackendStablePath(virStoragePoolObjPtr pool,
const char *devpath,
bool loop);
--
2.1.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v4 4/4] scsi: Adjust return values from processLU
https://bugzilla.redhat.com/show_bug.cgi?id=1171933
Adjust the processLU error returns to be a bit more logical. Currently,
the calling code cannot determine the difference between a non disk/lun
volume and a processed/found disk/lun. It can also not differentiate
between perhaps real/fatal error and one that won't necessarily stop
the code from finding other volumes.
After this patch virStorageBackendSCSIFindLUsInternal will stop processing
as soon as a fatal message occurs rather than continuting processing
for no apparent reason. It will also only set the *found value when
at least one of the processLU's was successful.
With the failed return, if the reason for the stop was that the pool
target path did not exist, was /dev, was /dev/, or did not start with
/dev, then iSCSI pool startup and refresh will fail.
Signed-off-by: John Ferlan jfer...@redhat.com
---
src/storage/storage_backend_scsi.c | 38 +-
1 file changed, 25 insertions(+), 13 deletions(-)
diff --git a/src/storage/storage_backend_scsi.c
b/src/storage/storage_backend_scsi.c
index b98311c..9b1f6a9 100644
--- a/src/storage/storage_backend_scsi.c
+++ b/src/storage/storage_backend_scsi.c
@@ -373,6 +373,15 @@ getBlockDevice(uint32_t host,
}
+/*
+ * Process a Logical Unit entry from the scsi host device directory
+ *
+ * Returns:
+ *
+ * 0 = Found a valid entry
+ * -1 = Some sort of fatal error
+ * -2 = non-fatal error or a non-disk entry
+ */
static int
processLU(virStoragePoolObjPtr pool,
uint32_t host,
@@ -391,7 +400,7 @@ processLU(virStoragePoolObjPtr pool,
virReportError(VIR_ERR_INTERNAL_ERROR,
_(Failed to determine if %u:%u:%u:%u is a
Direct-Access LUN),
host, bus, target, lun);
-goto out;
+return -1;
}
/* We don't create volumes for devices other than disk and cdrom
@@ -399,32 +408,28 @@ processLU(virStoragePoolObjPtr pool,
* isn't an error, either. */
if (!(device_type == VIR_STORAGE_DEVICE_TYPE_DISK ||
device_type == VIR_STORAGE_DEVICE_TYPE_ROM))
-{
-retval = 0;
-goto out;
-}
+return -2;
VIR_DEBUG(%u:%u:%u:%u is a Direct-Access LUN,
host, bus, target, lun);
if (getBlockDevice(host, bus, target, lun, block_device) 0) {
VIR_DEBUG(Failed to find block device for this LUN);
-goto out;
+return -1;
}
-if (virStorageBackendSCSINewLun(pool,
-host, bus, target, lun,
-block_device) 0) {
+retval = virStorageBackendSCSINewLun(pool, host, bus, target, lun,
+ block_device);
+if (retval 0) {
VIR_DEBUG(Failed to create new storage volume for %u:%u:%u:%u,
host, bus, target, lun);
-goto out;
+goto cleanup;
}
-retval = 0;
VIR_DEBUG(Created new storage volume for %u:%u:%u:%u successfully,
host, bus, target, lun);
- out:
+ cleanup:
VIR_FREE(block_device);
return retval;
}
@@ -457,6 +462,8 @@ virStorageBackendSCSIFindLUs(virStoragePoolObjPtr pool,
snprintf(devicepattern, sizeof(devicepattern), %u:%%u:%%u:%%u\n,
scanhost);
while ((retval = virDirRead(devicedir, lun_dirent, device_path)) 0) {
+int rc;
+
if (sscanf(lun_dirent-d_name, devicepattern,
bus, target, lun) != 3) {
continue;
@@ -464,7 +471,12 @@ virStorageBackendSCSIFindLUs(virStoragePoolObjPtr pool,
VIR_DEBUG(Found possible LU '%s', lun_dirent-d_name);
-if (processLU(pool, scanhost, bus, target, lun) == 0)
+rc = processLU(pool, scanhost, bus, target, lun);
+if (rc == -1) {
+retval = -1;
+break;
+}
+if (rc == 0)
found++;
}
--
2.1.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v4 0/4] storage: handle scsi/iscsi error paths better
v3 here: http://www.redhat.com/archives/libvir-list/2015-April/msg00240.html changes: Patch 1: (prior patch 1) Adjustments from code review - mostly just creating a new function using the const char * path instead of pool pointer. Called only from virStorageBackendStablePath Patch 2: (prior patch 3) Rather than continue to try to talk through the v3, here's the latest changes. These will just check in the SCSINewLun if the source pool target path doesn't start with /dev, then to just fail. This includes if the path is /dev or /dev/. Theory for failure is that even if we allowed /dev or /dev/ to continue down into the call to virStorageBackendStablePath all we'd get back was the duplicated 'devpath' which we'd claim was non-fatal. Patch 3: (NEW) Adjust virStorageBackendSCSIFindLUs to return a count of LU's found rather than the current 0 or -1 with a setting of the boolean found (which gets ignored in most cases). By returning a count, 0, or -1 the caller can decide what to do with the data. Patch 4: (prior patch 4) Couple of minor changes regarding comments and the use of a goto instead of if then else in processLU retval checks. Kept the flow as previous including using 'retval' in virStorageBackendSCSIFindLUs rather than creating yet another local status that would need to be checked. John Ferlan (4): storage: Split out the valid path check scsi: Adjust return value for virStorageBackendSCSINewLun scsi: Change return values for virStorageBackendSCSIFindLUs scsi: Adjust return values from processLU src/storage/storage_backend.c | 26 +- src/storage/storage_backend.h | 1 + src/storage/storage_backend_scsi.c | 101 - 3 files changed, 79 insertions(+), 49 deletions(-) -- 2.1.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v4 2/4] scsi: Adjust return value for virStorageBackendSCSINewLun
Use virStorageBackendPoolUseDevPath API to determine whether creation of
stable target path is possible for the volume.
This will differentiate a failed virStorageBackendStablePath which won't
need to be fatal. Thus, we'll add a -2 return value to differentiate that
the failure was a result of either the inability to find the symlink for
the device or failure to open the target path directory
Signed-off-by: John Ferlan jfer...@redhat.com
---
src/storage/storage_backend_scsi.c | 29 ++---
1 file changed, 26 insertions(+), 3 deletions(-)
diff --git a/src/storage/storage_backend_scsi.c
b/src/storage/storage_backend_scsi.c
index b96caec..ae3cd9a 100644
--- a/src/storage/storage_backend_scsi.c
+++ b/src/storage/storage_backend_scsi.c
@@ -146,6 +146,16 @@ virStorageBackendSCSISerial(const char *dev)
}
+/*
+ * Attempt to create a new LUN
+ *
+ * Returns:
+ *
+ * 0 = Success
+ * -1 = Failure due to some sort of OOM or other fatal issue found when
+ *attempting to get/update information about a found volume
+ * -2 = Failure to find a stable path, not fatal, caller can try another
+ */
static int
virStorageBackendSCSINewLun(virStoragePoolObjPtr pool,
uint32_t host ATTRIBUTE_UNUSED,
@@ -158,6 +168,20 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool,
char *devpath = NULL;
int retval = -1;
+/* Check if the pool is using a stable target path. The call to
+ * virStorageBackendStablePath will fail if the pool target path
+ * isn't stable and just return the strdup'd 'devpath' anyway.
+ * This would be indistinguishable to failing to find the stable
+ * path to the device if the virDirRead loop to search the
+ * target pool path for our devpath had failed.
+ */
+if (!virStorageBackendPoolPathIsStable(pool-def-target.path)) {
+virReportError(VIR_ERR_INVALID_ARG,
+ _(unable to use target path '%s' for dev '%s'),
+ NULLSTR(pool-def-target.path), dev);
+goto cleanup;
+}
+
if (VIR_ALLOC(vol) 0)
goto cleanup;
@@ -187,13 +211,12 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool,
true)) == NULL)
goto cleanup;
-if (STREQ(devpath, vol-target.path)
-!(STREQ(pool-def-target.path, /dev) ||
- STREQ(pool-def-target.path, /dev/))) {
+if (STREQ(devpath, vol-target.path)) {
VIR_DEBUG(No stable path found for '%s' in '%s',
devpath, pool-def-target.path);
+retval = -2;
goto cleanup;
}
--
2.1.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: lifecycle: guest get rebooted after reboot and shutdown
On 2015/4/17 17:54, Michal Privoznik wrote: On 17.04.2015 02:43, zhang bo wrote: Maybe I should have been more specific when requiring you to send the patch. If you look at 'git log' you'll see this commit message diverges from the rest. I've fixed it, ACKed and pushed. Michal Sorry for the inconvenience I brought in and Thank you very much!! :) Oscar -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
