[libvirt] [PATCH 0/8] Converge Storage Pool vHBA logic with Node Device
Similar to the recent node device focused vHBA patches, but these focus more on adjustments to the Storage Pool vHBA logic and finally the mechanism to define a vHBA for a domain. The first patch fixes a leak found by coverity that showed up because node_device_conf had enough changes so that coverity looked harder... The second patch creates a mechanism to mock creation of the vHBA in order to test the ability for the storage pool to create a vHBA. As much as I dislike forward refs for testNodeDeviceMockCreateVport, it was better than moving all the code.... The third patch extracts out storage device mgmt into it's own set of src/util API's - similar to the existing virstoragefile, but for devices. The fourth patch was uncovered while moving code from storage_backend_scsi into node_device_conf (the fifth patch)... The fifth patch moves the createVport/deleteVport guts into the node_device_conf (although they could have moved to virvhba)... The sixth patch alters the logic to use the node_device API's as the "preferred" mechanism to create/delete the vport... The seventh patch tests the storage pool vHBA creation algorithms. The eigth patch is the reason for all this stirring of the pot. Alter the domain <controller> XML in order to allow definition of a vHBA which more or less sits between a "scsi_hostX" host device and a controller. This is in preparation for https://bugzilla.redhat.com/show_bug.cgi?id=1404962 which can take that created vHBA and automagically add the LUNs from the vHBA to the domain although that requires a bit more magic for which there are already onlist patches to let qemu driver know when a node device has been added/removed. Once all that's in place - the next step will be to converge the two sets of patches. It's a chicken/egg type problem - one has to exist before the other can truly work. John Ferlan (8): conf: Fix leak in virNodeDeviceDefParseXML tests: Add createVHBAByStoragePool-by-parent to fchosttest util: Convert virStoragePoolSourceAdapter to virStorageAdapter util: Rename virFileWaitForDevices storage: Move/rename createVport and deleteVport util: Alter virNodeDevice{Create|Delete}Vport to use nodedev APIs tests: Add more storage pool vHBA tests conf: Add vHBA controller definition to domain docs/schemas/basictypes.rng | 66 ++-- docs/schemas/domaincommon.rng | 12 +- po/POTFILES.in | 1 + src/Makefile.am | 1 + src/conf/domain_audit.c | 32 ++ src/conf/domain_conf.c | 180 ++++++++++- src/conf/domain_conf.h | 2 + src/conf/node_device_conf.c | 342 ++++++++++++++++++++- src/conf/node_device_conf.h | 9 + src/conf/storage_conf.c | 338 +++++--------------- src/conf/storage_conf.h | 35 +-- src/libvirt_private.syms | 20 +- src/libxl/libxl_conf.c | 1 + src/node_device/node_device_driver.c | 2 +- src/phyp/phyp_driver.c | 3 +- src/qemu/qemu_alias.c | 5 + src/qemu/qemu_command.c | 4 + src/qemu/qemu_hotplug.c | 16 + src/storage/storage_backend_disk.c | 6 +- src/storage/storage_backend_iscsi.c | 2 +- src/storage/storage_backend_logical.c | 4 +- src/storage/storage_backend_mpath.c | 2 +- src/storage/storage_backend_scsi.c | 267 +++------------- src/storage/storage_util.c | 2 +- src/test/test_driver.c | 101 +++++- src/util/virfile.h | 2 - src/util/virscsihost.c | 28 +- src/util/virscsihost.h | 8 +- src/util/virstoragedevice.c | 292 ++++++++++++++++++ src/util/virstoragedevice.h | 89 ++++++ src/util/virutil.c | 4 +- src/util/virutil.h | 2 + tests/fchosttest.c | 111 +++++++ .../qemuxml2argv-vhba-no-parent.xml | 38 +++ .../qemuxml2argv-vhba-parent-fabric.xml | 38 +++ .../qemuxml2argv-vhba-parent-name.xml | 38 +++ .../qemuxml2argv-vhba-parent-wwns.xml | 38 +++ .../qemuxml2xmlout-vhba-no-parent.xml | 47 +++ .../qemuxml2xmlout-vhba-parent-fabric.xml | 47 +++ .../qemuxml2xmlout-vhba-parent-name.xml | 47 +++ .../qemuxml2xmlout-vhba-parent-wwns.xml | 47 +++ tests/qemuxml2xmltest.c | 9 + 42 files changed, 1727 insertions(+), 611 deletions(-) create mode 100644 src/util/virstoragedevice.c create mode 100644 src/util/virstoragedevice.h create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-vhba-no-parent.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-vhba-parent-fabric.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-vhba-parent-name.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-vhba-parent-wwns.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-vhba-no-parent.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-vhba-parent-fabric.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-vhba-parent-name.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-vhba-parent-wwns.xml -- 2.9.3
The 'nodes' is overwritten after the first usage and possibly leaked if any code in the first set of parsing goes to error. Found by Coverity. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/node_device_conf.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index 1c81b48..07b0838 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -1766,7 +1766,7 @@ virNodeDeviceDefParseXML(xmlXPathContextPtr ctxt, { virNodeDeviceDefPtr def; virNodeDevCapsDefPtr *next_cap; - xmlNodePtr *nodes; + xmlNodePtr *nodes = NULL; int n, m; size_t i; @@ -1789,7 +1789,6 @@ virNodeDeviceDefParseXML(xmlXPathContextPtr ctxt, def->sysfs_path = virXPathString("string(./path[1])", ctxt); /* Parse devnodes */ - nodes = NULL; if ((n = virXPathNodeSet("./devnode", ctxt, &nodes)) < 0) goto error; @@ -1842,7 +1841,7 @@ virNodeDeviceDefParseXML(xmlXPathContextPtr ctxt, ctxt); /* Parse device capabilities */ - nodes = NULL; + VIR_FREE(nodes); if ((n = virXPathNodeSet("./capability", ctxt, &nodes)) < 0) goto error; @@ -1859,10 +1858,8 @@ virNodeDeviceDefParseXML(xmlXPathContextPtr ctxt, nodes[i], create, virt_type); - if (!*next_cap) { - VIR_FREE(nodes); + if (!*next_cap) goto error; - } next_cap = &(*next_cap)->next; } @@ -1872,6 +1869,7 @@ virNodeDeviceDefParseXML(xmlXPathContextPtr ctxt, error: virNodeDeviceDefFree(def); + VIR_FREE(nodes); return NULL; } -- 2.9.3
On 20.02.2017 14:18, John Ferlan wrote:
The 'nodes' is overwritten after the first usage and possibly leaked if any code in the first set of parsing goes to error.
Found by Coverity.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/node_device_conf.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)
ACK and safe for the freeze. Michal
Add a new test to fchosttest in order to test creation of our vHBA via the Storage Pool logic. Unlike the real code, we cannot yet use the virVHBA* API's because they (currently) traverse the file system in order to get the parent vport capable scsi_host. Besides there's no "real" NPIV device here - so we have to take some liberties, at least for now. Instead, we'll follow the node device tests partially in order to create and destroy the vHBA with the test node devices. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/test/test_driver.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++++-- tests/fchosttest.c | 64 ++++++++++++++++++++++++++++++++++ 2 files changed, 157 insertions(+), 2 deletions(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 5fef3f1..4dff0f1 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -4365,6 +4365,32 @@ testConnectFindStoragePoolSources(virConnectPtr conn ATTRIBUTE_UNUSED, } +static virNodeDeviceDefPtr +testNodeDeviceMockCreateVport(virConnectPtr conn, + const char *wwnn, + const char *wwpn); +static int +testCreateVport(virConnectPtr conn, + const char *wwnn, + const char *wwpn) +{ + /* The storage_backend_scsi createVport() will use the input adapter + * fields parent name, parent_wwnn/parent_wwpn, or parent_fabric_wwn + * in order to determine whether the provided parent can be used to + * create a vHBA or will find "an available vport capable" to create + * a vHBA. In order to do this, it uses the virVHBA* API's which traverse + * the sysfs looking at various fields (rather than going via nodedev). + * + * Since the test environ doesn't have the sysfs for the storage pool + * test, at least for now use the node device test infrastructure to + * create the vHBA. In the long run the result is the same. */ + if (!testNodeDeviceMockCreateVport(conn, wwnn, wwpn)) + return -1; + + return 0; +} + + static virStoragePoolPtr testStoragePoolCreateXML(virConnectPtr conn, const char *xml, @@ -4395,6 +4421,24 @@ testStoragePoolCreateXML(virConnectPtr conn, goto cleanup; def = NULL; + if (pool->def->source.adapter.type == + VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) { + /* In the real code, we'd call virVHBAManageVport followed by + * find_new_device, but we cannot do that here since we're not + * mocking udev. The mock routine will copy an existing vHBA and + * rename a few fields to mock that. So in order to allow that to + * work properly, we need to drop our lock */ + testDriverUnlock(privconn); + if (testCreateVport(conn, pool->def->source.adapter.data.fchost.wwnn, + pool->def->source.adapter.data.fchost.wwpn) < 0) { + virStoragePoolObjRemove(&privconn->pools, pool); + pool = NULL; + testDriverLock(privconn); + goto cleanup; + } + testDriverLock(privconn); + } + if (testStoragePoolObjSetDefaults(pool) == -1) { virStoragePoolObjRemove(&privconn->pools, pool); pool = NULL; @@ -4405,7 +4449,6 @@ testStoragePoolCreateXML(virConnectPtr conn, event = virStoragePoolEventLifecycleNew(pool->def->name, pool->def->uuid, VIR_STORAGE_POOL_EVENT_STARTED, 0); - ret = virGetStoragePool(conn, pool->def->name, pool->def->uuid, NULL, NULL); @@ -4539,6 +4582,44 @@ testStoragePoolBuild(virStoragePoolPtr pool, static int +testDestroyVport(testDriverPtr privconn, + const char *wwnn ATTRIBUTE_UNUSED, + const char *wwpn ATTRIBUTE_UNUSED) +{ + int ret = -1; + virNodeDeviceObjPtr obj = NULL; + virObjectEventPtr event = NULL; + + /* NB: Cannot use virVHBAGetHostByWWN (yet) like the storage_backend_scsi + * deleteVport() helper since that traverses the file system looking for + * the wwnn/wwpn. So our choice short term is to cheat and use the name + * (scsi_host12) we know was created. + * + * Reaching across the boundaries of space and time into the + * Node Device in order to remove */ + if (!(obj = virNodeDeviceFindByName(&privconn->devs, "scsi_host12"))) { + virReportError(VIR_ERR_NO_NODE_DEVICE, "%s", + _("no node device with matching name 'scsi_host12'")); + goto cleanup; + } + + event = virNodeDeviceEventLifecycleNew("scsi_host12", + VIR_NODE_DEVICE_EVENT_DELETED, + 0); + + virNodeDeviceObjRemove(&privconn->devs, &obj); + + ret = 0; + + cleanup: + if (obj) + virNodeDeviceObjUnlock(obj); + testObjectEventQueue(privconn, event); + return ret; +} + + +static int testStoragePoolDestroy(virStoragePoolPtr pool) { testDriverPtr privconn = pool->conn->privateData; @@ -4562,7 +4643,17 @@ testStoragePoolDestroy(virStoragePoolPtr pool) } privpool->active = 0; - event = virStoragePoolEventLifecycleNew(privpool->def->name, privpool->def->uuid, + + if (privpool->def->source.adapter.type == + VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) { + if (testDestroyVport(privconn, + privpool->def->source.adapter.data.fchost.wwnn, + privpool->def->source.adapter.data.fchost.wwpn) < 0) + goto cleanup; + } + + event = virStoragePoolEventLifecycleNew(privpool->def->name, + privpool->def->uuid, VIR_STORAGE_POOL_EVENT_STOPPED, 0); diff --git a/tests/fchosttest.c b/tests/fchosttest.c index 51fdcbd..dc6b9af 100644 --- a/tests/fchosttest.c +++ b/tests/fchosttest.c @@ -75,6 +75,19 @@ static const char test10_xml[] = " </capability>" "</device>"; +/* virStoragePoolCreateXML using parent='%s' to find the vport capable HBA */ +static const char test11_xml[] = +"<pool type='scsi'>" +" <name>vhba_pool</name>" +" <source>" +" <adapter type='fc_host' parent='scsi_host1' wwnn='20000000c9831b4b' wwpn='10000000c9831b4b'/>" +" </source>" +" <target>" +" <path>/dev/disk/by-path</path>" +" </target>" +"</pool>"; + + /* Test virIsVHBACapable */ static int test1(const void *data ATTRIBUTE_UNUSED) @@ -275,6 +288,54 @@ manageVHBAByNodeDevice(const void *data) } +/* Test manageVHBAByStoragePool + * - Test both virStoragePoolCreateXML and virStoragePoolDestroy + * - Create a storage pool vHBA allowing usage of various different + * methods based on the input data/xml argument. + * - Be sure that it's possible to destroy the storage pool as well. + */ +static int +manageVHBAByStoragePool(const void *data) +{ + const char *expect_hostname = "scsi_host12"; + virConnectPtr conn = NULL; + virStoragePoolPtr pool = NULL; + virNodeDevicePtr dev = NULL; + int ret = -1; + const char *vhba = data; + + if (!(conn = virConnectOpen("test:///default"))) + return -1; + + if (!(pool = virStoragePoolCreateXML(conn, vhba, 0))) + goto cleanup; + + if (!(dev = virNodeDeviceLookupByName(conn, expect_hostname))) { + VIR_DEBUG("Failed to find expected_hostname '%s'", expect_hostname); + ignore_value(virStoragePoolDestroy(pool)); + goto cleanup; + } + + if (virStoragePoolDestroy(pool) < 0) + goto cleanup; + + if ((dev = virNodeDeviceLookupByName(conn, expect_hostname))) { + VIR_DEBUG("Found expected_hostname '%s' after destroy", + expect_hostname); + goto cleanup; + } + + ret = 0; + + cleanup: + if (pool) + virStoragePoolFree(pool); + if (conn) + virConnectClose(conn); + return ret; +} + + static int mymain(void) { @@ -310,6 +371,9 @@ mymain(void) if (virTestRun("manageVHBAByNodeDevice-parent-fabric-wwn", manageVHBAByNodeDevice, test10_xml) < 0) ret = -1; + if (virTestRun("manageVHBAByStoragePool-by-parent", manageVHBAByStoragePool, + test11_xml) < 0) + ret = -1; cleanup: VIR_FREE(fchost_prefix); -- 2.9.3
On 20.02.2017 14:18, John Ferlan wrote:
Add a new test to fchosttest in order to test creation of our vHBA via the Storage Pool logic. Unlike the real code, we cannot yet use the virVHBA* API's because they (currently) traverse the file system in order to get the parent vport capable scsi_host. Besides there's no "real" NPIV device here - so we have to take some liberties, at least for now.
Instead, we'll follow the node device tests partially in order to create and destroy the vHBA with the test node devices.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/test/test_driver.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++++-- tests/fchosttest.c | 64 ++++++++++++++++++++++++++++++++++ 2 files changed, 157 insertions(+), 2 deletions(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 5fef3f1..4dff0f1 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -4365,6 +4365,32 @@ testConnectFindStoragePoolSources(virConnectPtr conn ATTRIBUTE_UNUSED, }
+static virNodeDeviceDefPtr +testNodeDeviceMockCreateVport(virConnectPtr conn, + const char *wwnn, + const char *wwpn); +static int +testCreateVport(virConnectPtr conn, + const char *wwnn, + const char *wwpn) +{ + /* The storage_backend_scsi createVport() will use the input adapter + * fields parent name, parent_wwnn/parent_wwpn, or parent_fabric_wwn + * in order to determine whether the provided parent can be used to + * create a vHBA or will find "an available vport capable" to create + * a vHBA. In order to do this, it uses the virVHBA* API's which traverse + * the sysfs looking at various fields (rather than going via nodedev). + * + * Since the test environ doesn't have the sysfs for the storage pool + * test, at least for now use the node device test infrastructure to + * create the vHBA. In the long run the result is the same. */ + if (!testNodeDeviceMockCreateVport(conn, wwnn, wwpn)) + return -1; + + return 0; +} + + static virStoragePoolPtr testStoragePoolCreateXML(virConnectPtr conn, const char *xml, @@ -4395,6 +4421,24 @@ testStoragePoolCreateXML(virConnectPtr conn, goto cleanup; def = NULL;
+ if (pool->def->source.adapter.type == + VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) { + /* In the real code, we'd call virVHBAManageVport followed by + * find_new_device, but we cannot do that here since we're not + * mocking udev. The mock routine will copy an existing vHBA and + * rename a few fields to mock that. So in order to allow that to + * work properly, we need to drop our lock */ + testDriverUnlock(privconn); + if (testCreateVport(conn, pool->def->source.adapter.data.fchost.wwnn, + pool->def->source.adapter.data.fchost.wwpn) < 0) { + virStoragePoolObjRemove(&privconn->pools, pool); + pool = NULL; + testDriverLock(privconn); + goto cleanup; + } + testDriverLock(privconn);
So we need this testDriverLock() and Unlock() calls because testCreateVport() calls testNodeDeviceMockCreateVport() which then call top level APIs for looking up a nodedev and fetching its XML. Pardon my language but that looks stup^Wweird. Mind fixing that?
+ } +
Otherwise looking good. Michal
On 02/27/2017 10:36 AM, Michal Privoznik wrote:
On 20.02.2017 14:18, John Ferlan wrote:
Add a new test to fchosttest in order to test creation of our vHBA via the Storage Pool logic. Unlike the real code, we cannot yet use the virVHBA* API's because they (currently) traverse the file system in order to get the parent vport capable scsi_host. Besides there's no "real" NPIV device here - so we have to take some liberties, at least for now.
Instead, we'll follow the node device tests partially in order to create and destroy the vHBA with the test node devices.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/test/test_driver.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++++-- tests/fchosttest.c | 64 ++++++++++++++++++++++++++++++++++ 2 files changed, 157 insertions(+), 2 deletions(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 5fef3f1..4dff0f1 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -4365,6 +4365,32 @@ testConnectFindStoragePoolSources(virConnectPtr conn ATTRIBUTE_UNUSED, }
+static virNodeDeviceDefPtr +testNodeDeviceMockCreateVport(virConnectPtr conn, + const char *wwnn, + const char *wwpn); +static int +testCreateVport(virConnectPtr conn, + const char *wwnn, + const char *wwpn) +{ + /* The storage_backend_scsi createVport() will use the input adapter + * fields parent name, parent_wwnn/parent_wwpn, or parent_fabric_wwn + * in order to determine whether the provided parent can be used to + * create a vHBA or will find "an available vport capable" to create + * a vHBA. In order to do this, it uses the virVHBA* API's which traverse + * the sysfs looking at various fields (rather than going via nodedev). + * + * Since the test environ doesn't have the sysfs for the storage pool + * test, at least for now use the node device test infrastructure to + * create the vHBA. In the long run the result is the same. */ + if (!testNodeDeviceMockCreateVport(conn, wwnn, wwpn)) + return -1; + + return 0; +} + + static virStoragePoolPtr testStoragePoolCreateXML(virConnectPtr conn, const char *xml, @@ -4395,6 +4421,24 @@ testStoragePoolCreateXML(virConnectPtr conn, goto cleanup; def = NULL;
+ if (pool->def->source.adapter.type == + VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) { + /* In the real code, we'd call virVHBAManageVport followed by + * find_new_device, but we cannot do that here since we're not + * mocking udev. The mock routine will copy an existing vHBA and + * rename a few fields to mock that. So in order to allow that to + * work properly, we need to drop our lock */ + testDriverUnlock(privconn); + if (testCreateVport(conn, pool->def->source.adapter.data.fchost.wwnn, + pool->def->source.adapter.data.fchost.wwpn) < 0) { + virStoragePoolObjRemove(&privconn->pools, pool); + pool = NULL; + testDriverLock(privconn); + goto cleanup; + } + testDriverLock(privconn);
So we need this testDriverLock() and Unlock() calls because testCreateVport() calls testNodeDeviceMockCreateVport() which then call top level APIs for looking up a nodedev and fetching its XML. Pardon my language but that looks stup^Wweird. Mind fixing that?
Well it does follow similar ugliness in testNodeDeviceCreateXML Somewhat ironically I have an RFC series posted that can reduce/remove the need a well, but it's quite a bit more change... It also modifies nodedev's to use hash tables instead of (long) linked lists that are currently being used. With that series in place, this ugliness wouldn't be needed. Anyway, so as an adjustment at least here... I could move the hunk below the pool->active = 1 and before the event. Then drop the lock entirely before call the testCreateVport. Would also need to add a 'isLocked' so that the unlock isn't called unnecessarily. Of course that's almost as equally as ugly. Unless you had another methodology in mind... John
+ } +
Otherwise looking good.
Michal
On 27.02.2017 17:50, John Ferlan wrote:
On 02/27/2017 10:36 AM, Michal Privoznik wrote:
On 20.02.2017 14:18, John Ferlan wrote:
Add a new test to fchosttest in order to test creation of our vHBA via the Storage Pool logic. Unlike the real code, we cannot yet use the virVHBA* API's because they (currently) traverse the file system in order to get the parent vport capable scsi_host. Besides there's no "real" NPIV device here - so we have to take some liberties, at least for now.
Instead, we'll follow the node device tests partially in order to create and destroy the vHBA with the test node devices.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/test/test_driver.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++++-- tests/fchosttest.c | 64 ++++++++++++++++++++++++++++++++++ 2 files changed, 157 insertions(+), 2 deletions(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 5fef3f1..4dff0f1 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -4365,6 +4365,32 @@ testConnectFindStoragePoolSources(virConnectPtr conn ATTRIBUTE_UNUSED, }
+static virNodeDeviceDefPtr +testNodeDeviceMockCreateVport(virConnectPtr conn, + const char *wwnn, + const char *wwpn); +static int +testCreateVport(virConnectPtr conn, + const char *wwnn, + const char *wwpn) +{ + /* The storage_backend_scsi createVport() will use the input adapter + * fields parent name, parent_wwnn/parent_wwpn, or parent_fabric_wwn + * in order to determine whether the provided parent can be used to + * create a vHBA or will find "an available vport capable" to create + * a vHBA. In order to do this, it uses the virVHBA* API's which traverse + * the sysfs looking at various fields (rather than going via nodedev). + * + * Since the test environ doesn't have the sysfs for the storage pool + * test, at least for now use the node device test infrastructure to + * create the vHBA. In the long run the result is the same. */ + if (!testNodeDeviceMockCreateVport(conn, wwnn, wwpn)) + return -1; + + return 0; +} + + static virStoragePoolPtr testStoragePoolCreateXML(virConnectPtr conn, const char *xml, @@ -4395,6 +4421,24 @@ testStoragePoolCreateXML(virConnectPtr conn, goto cleanup; def = NULL;
+ if (pool->def->source.adapter.type == + VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) { + /* In the real code, we'd call virVHBAManageVport followed by + * find_new_device, but we cannot do that here since we're not + * mocking udev. The mock routine will copy an existing vHBA and + * rename a few fields to mock that. So in order to allow that to + * work properly, we need to drop our lock */ + testDriverUnlock(privconn); + if (testCreateVport(conn, pool->def->source.adapter.data.fchost.wwnn, + pool->def->source.adapter.data.fchost.wwpn) < 0) { + virStoragePoolObjRemove(&privconn->pools, pool); + pool = NULL; + testDriverLock(privconn); + goto cleanup; + } + testDriverLock(privconn);
So we need this testDriverLock() and Unlock() calls because testCreateVport() calls testNodeDeviceMockCreateVport() which then call top level APIs for looking up a nodedev and fetching its XML. Pardon my language but that looks stup^Wweird. Mind fixing that?
Well it does follow similar ugliness in testNodeDeviceCreateXML
Somewhat ironically I have an RFC series posted that can reduce/remove the need a well, but it's quite a bit more change... It also modifies nodedev's to use hash tables instead of (long) linked lists that are currently being used. With that series in place, this ugliness wouldn't be needed.
Anyway, so as an adjustment at least here... I could move the hunk below the pool->active = 1 and before the event. Then drop the lock entirely before call the testCreateVport. Would also need to add a 'isLocked' so that the unlock isn't called unnecessarily. Of course that's almost as equally as ugly.
Unless you had another methodology in mind...
What about these lines (in testNodeDeviceMockCreateVport): if (!(objcopy = virNodeDeviceFindByName(&driver->devs, "scsi_host11")) || !(xml = virNodeDeviceDefFormat(objcopy->def)) || !(def = virNodeDeviceDefParseString(xml, EXISTING_DEVICE, NULL))) goto cleanup; I haven't even compile-tested them, but in general - they call the important parts of the public APIs without us needing to lock()/unlock() the driver meanwhile. There might be some additional unlock(objcopy) required, unref() or free(), but I'm willing to do that if it saves us from weird driver lock()/unlock() calls. If you have this ^^ patch before this one, you can just drop test driver lock()/unlock() here. Michal
[...]
Anyway, so as an adjustment at least here... I could move the hunk below the pool->active = 1 and before the event. Then drop the lock entirely before call the testCreateVport. Would also need to add a 'isLocked' so that the unlock isn't called unnecessarily. Of course that's almost as equally as ugly.
Unless you had another methodology in mind...
What about these lines (in testNodeDeviceMockCreateVport):
if (!(objcopy = virNodeDeviceFindByName(&driver->devs, "scsi_host11")) || !(xml = virNodeDeviceDefFormat(objcopy->def)) || !(def = virNodeDeviceDefParseString(xml, EXISTING_DEVICE, NULL))) goto cleanup;
I haven't even compile-tested them, but in general - they call the important parts of the public APIs without us needing to lock()/unlock() the driver meanwhile. There might be some additional unlock(objcopy) required, unref() or free(), but I'm willing to do that if it saves us from weird driver lock()/unlock() calls.
If you have this ^^ patch before this one, you can just drop test driver lock()/unlock() here.
Michal
The above doesn't work as cleanly as one would hope as eventtest hangs, but after a bit of finagling, the following works: if (!(objcpy = virNodeDeviceFindByName(&driver->devs, "scsi_host11"))) goto cleanup; xml = virNodeDeviceDefFormat(objcpy->def); virNodeDeviceObjUnlock(objcpy); if (!xml) goto cleanup; if (!(def = virNodeDeviceDefParseString(xml, EXISTING_DEVICE, NULL))) goto cleanup; Going this route also removes the need for the existing caller to do unlock/lock game as well. John FWIW: The lock gets easier with RFC series and of course that's in the back of my mind every time I touch this code... All the fun I'll have merging changes...
On 02/27/2017 11:05 PM, John Ferlan wrote:
[...]
Anyway, so as an adjustment at least here... I could move the hunk below the pool->active = 1 and before the event. Then drop the lock entirely before call the testCreateVport. Would also need to add a 'isLocked' so that the unlock isn't called unnecessarily. Of course that's almost as equally as ugly.
Unless you had another methodology in mind...
What about these lines (in testNodeDeviceMockCreateVport):
if (!(objcopy = virNodeDeviceFindByName(&driver->devs, "scsi_host11")) || !(xml = virNodeDeviceDefFormat(objcopy->def)) || !(def = virNodeDeviceDefParseString(xml, EXISTING_DEVICE, NULL))) goto cleanup;
I haven't even compile-tested them, but in general - they call the important parts of the public APIs without us needing to lock()/unlock() the driver meanwhile. There might be some additional unlock(objcopy) required, unref() or free(), but I'm willing to do that if it saves us from weird driver lock()/unlock() calls.
If you have this ^^ patch before this one, you can just drop test driver lock()/unlock() here.
Michal
The above doesn't work as cleanly as one would hope as eventtest hangs, but after a bit of finagling, the following works:
Ah, now I see why it hangs. The problem lies elsewhere: testNodeDeviceCreateXML() calls public APIs thus it unlocks the driver again. I mean, testNodeDeviceMockCreateVport() is not the only function it calls with unlocked driver. Le sigh.
if (!(objcpy = virNodeDeviceFindByName(&driver->devs, "scsi_host11"))) goto cleanup;
xml = virNodeDeviceDefFormat(objcpy->def); virNodeDeviceObjUnlock(objcpy); if (!xml) goto cleanup;
if (!(def = virNodeDeviceDefParseString(xml, EXISTING_DEVICE, NULL))) goto cleanup;
Yup, I've expected that we will need to unlock the object returned by virNodeDeviceFindByName(). This looks much nicer IMO. But we still need to fix testNodeDeviceCreateXML(). Working on the fix now.
Going this route also removes the need for the existing caller to do unlock/lock game as well.
John
FWIW: The lock gets easier with RFC series and of course that's in the back of my mind every time I touch this code... All the fun I'll have merging changes...
Ah, which patches are that? I want to review them. Michal
FWIW: The lock gets easier with RFC series and of course that's in the back of my mind every time I touch this code... All the fun I'll have merging changes...
Ah, which patches are that? I want to review them.
Michal
I need to post an update since nodedev and storage changes have caused merge conflicts, but the initial version is: http://www.redhat.com/archives/libvir-list/2017-February/msg00519.html Essentially for nodedev, interface, nwfilter, secret, volume, storage, and network I've attempted to commonalize the hash table logic from domain (and domain snapshot). Each of the drivers ends up then having a very common look and feel. Unfortunately they're not simple to look at. John
Introduce src/util/virstoragedevice.{c,h}. Rather than have virstoragefile be a repository for more things - create a new module to share the adapter definitions between the storage pool and eventually the domain. The devices will need device address functions (no need to pollute more code for that). Move the virStoragePoolSourceAdapter from storage_conf into the new module and parcel out the the structure a bit more into 'fchost' and 'scsi_host' specific pieces creating virStoragePoolSourceAdapterSCSIHost and virStoragePoolSourceAdapterFCHost structures for easier access. Modify code in the various places which formerly used the pool structure to use the new model. This includes some changes that will use the Ptr rather than just the struct (try to shorten the number of times one has to type adapter.data.fchost or adapter.data.scsi_host as well as [pool->]def->source.adapter. Signed-off-by: John Ferlan <jferlan@redhat.com> --- po/POTFILES.in | 1 + src/Makefile.am | 1 + src/conf/storage_conf.c | 338 ++++++++----------------------------- src/conf/storage_conf.h | 35 +--- src/libvirt_private.syms | 16 +- src/libxl/libxl_conf.c | 1 + src/phyp/phyp_driver.c | 3 +- src/storage/storage_backend_scsi.c | 131 +++++++------- src/test/test_driver.c | 4 +- src/util/virscsihost.c | 28 +-- src/util/virscsihost.h | 8 +- src/util/virstoragedevice.c | 292 ++++++++++++++++++++++++++++++++ src/util/virstoragedevice.h | 89 ++++++++++ 13 files changed, 548 insertions(+), 399 deletions(-) create mode 100644 src/util/virstoragedevice.c create mode 100644 src/util/virstoragedevice.h diff --git a/po/POTFILES.in b/po/POTFILES.in index 9f66697..fe07d16 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -248,6 +248,7 @@ src/util/virsecret.c src/util/virsexpr.c src/util/virsocketaddr.c src/util/virstorageencryption.c +src/util/virstoragedevice.c src/util/virstoragefile.c src/util/virstring.c src/util/virsysinfo.c diff --git a/src/Makefile.am b/src/Makefile.am index 46ca272..d14b376 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -171,6 +171,7 @@ UTIL_SOURCES = \ util/virsexpr.c util/virsexpr.h \ util/virsocketaddr.h util/virsocketaddr.c \ util/virstorageencryption.c util/virstorageencryption.h \ + util/virstoragedevice.c util/virstoragedevice.h \ util/virstoragefile.c util/virstoragefile.h \ util/virstring.h util/virstring.c \ util/virsysinfo.c util/virsysinfo.h \ diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 0e9a51f..c68e827 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -100,10 +100,6 @@ VIR_ENUM_IMPL(virStoragePartedFs, "ext2", "ext2", "extended") -VIR_ENUM_IMPL(virStoragePoolSourceAdapter, - VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_LAST, - "default", "scsi_host", "fc_host") - typedef const char *(*virStorageVolFormatToString)(int format); typedef int (*virStorageVolFormatFromString)(const char *format); @@ -342,21 +338,6 @@ virStorageVolDefFree(virStorageVolDefPtr def) VIR_FREE(def); } -static void -virStoragePoolSourceAdapterClear(virStoragePoolSourceAdapterPtr adapter) -{ - if (adapter->type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) { - VIR_FREE(adapter->data.fchost.wwnn); - VIR_FREE(adapter->data.fchost.wwpn); - VIR_FREE(adapter->data.fchost.parent); - VIR_FREE(adapter->data.fchost.parent_wwnn); - VIR_FREE(adapter->data.fchost.parent_wwpn); - VIR_FREE(adapter->data.fchost.parent_fabric_wwn); - } else if (adapter->type == - VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) { - VIR_FREE(adapter->data.scsi_host.name); - } -} void virStoragePoolSourceDeviceClear(virStoragePoolSourceDevicePtr dev) @@ -382,7 +363,7 @@ virStoragePoolSourceClear(virStoragePoolSourcePtr source) VIR_FREE(source->devices); VIR_FREE(source->dir); VIR_FREE(source->name); - virStoragePoolSourceAdapterClear(&source->adapter); + virStorageAdapterClear(&source->adapter); VIR_FREE(source->initiator.iqn); virStorageAuthDefFree(source->auth); VIR_FREE(source->vendor); @@ -462,6 +443,7 @@ virStoragePoolObjRemove(virStoragePoolObjListPtr pools, } } + static int virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, virStoragePoolSourcePtr source, @@ -470,14 +452,13 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, { int ret = -1; xmlNodePtr relnode, authnode, *nodeset = NULL; + xmlNodePtr adapternode; int nsource; size_t i; virStoragePoolOptionsPtr options; virStorageAuthDefPtr authdef = NULL; char *name = NULL; char *port = NULL; - char *adapter_type = NULL; - char *managed = NULL; int n; relnode = ctxt->node; @@ -583,109 +564,9 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, VIR_STRDUP(source->dir, "/") < 0) goto cleanup; - if ((adapter_type = virXPathString("string(./adapter/@type)", ctxt))) { - if ((source->adapter.type = - virStoragePoolSourceAdapterTypeFromString(adapter_type)) <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Unknown pool adapter type '%s'"), - adapter_type); + if ((adapternode = virXPathNode("./adapter", ctxt))) { + if (virStorageAdapterParseXML(&source->adapter, adapternode, ctxt) < 0) goto cleanup; - } - - if (source->adapter.type == - VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) { - source->adapter.data.fchost.parent = - virXPathString("string(./adapter/@parent)", ctxt); - managed = virXPathString("string(./adapter/@managed)", ctxt); - if (managed) { - source->adapter.data.fchost.managed = - virTristateBoolTypeFromString(managed); - if (source->adapter.data.fchost.managed < 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown fc_host managed setting '%s'"), - managed); - goto cleanup; - } - } - - source->adapter.data.fchost.parent_wwnn = - virXPathString("string(./adapter/@parent_wwnn)", ctxt); - source->adapter.data.fchost.parent_wwpn = - virXPathString("string(./adapter/@parent_wwpn)", ctxt); - source->adapter.data.fchost.parent_fabric_wwn = - virXPathString("string(./adapter/@parent_fabric_wwn)", ctxt); - - source->adapter.data.fchost.wwpn = - virXPathString("string(./adapter/@wwpn)", ctxt); - source->adapter.data.fchost.wwnn = - virXPathString("string(./adapter/@wwnn)", ctxt); - } else if (source->adapter.type == - VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) { - - source->adapter.data.scsi_host.name = - virXPathString("string(./adapter/@name)", ctxt); - if (virXPathNode("./adapter/parentaddr", ctxt)) { - xmlNodePtr addrnode = virXPathNode("./adapter/parentaddr/address", - ctxt); - virPCIDeviceAddressPtr addr = - &source->adapter.data.scsi_host.parentaddr; - - if (!addrnode) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("Missing scsi_host PCI address element")); - goto cleanup; - } - source->adapter.data.scsi_host.has_parent = true; - if (virPCIDeviceAddressParseXML(addrnode, addr) < 0) - goto cleanup; - if ((virXPathInt("string(./adapter/parentaddr/@unique_id)", - ctxt, - &source->adapter.data.scsi_host.unique_id) < 0) || - (source->adapter.data.scsi_host.unique_id < 0)) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("Missing or invalid scsi adapter " - "'unique_id' value")); - goto cleanup; - } - } - } - } else { - char *wwnn = NULL; - char *wwpn = NULL; - char *parent = NULL; - - /* "type" was not specified in the XML, so we must verify that - * "wwnn", "wwpn", "parent", or "parentaddr" are also not in the - * XML. If any are found, then we cannot just use "name" alone". - */ - wwnn = virXPathString("string(./adapter/@wwnn)", ctxt); - wwpn = virXPathString("string(./adapter/@wwpn)", ctxt); - parent = virXPathString("string(./adapter/@parent)", ctxt); - - if (wwnn || wwpn || parent) { - VIR_FREE(wwnn); - VIR_FREE(wwpn); - VIR_FREE(parent); - virReportError(VIR_ERR_XML_ERROR, "%s", - _("Use of 'wwnn', 'wwpn', and 'parent' attributes " - "requires use of the adapter 'type'")); - goto cleanup; - } - - if (virXPathNode("./adapter/parentaddr", ctxt)) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("Use of 'parent' element requires use " - "of the adapter 'type'")); - goto cleanup; - } - - /* To keep back-compat, 'type' is not required to specify - * for scsi_host adapter. - */ - if ((source->adapter.data.scsi_host.name = - virXPathString("string(./adapter/@name)", ctxt))) - source->adapter.type = - VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST; } if ((authnode = virXPathNode("./auth", ctxt))) { @@ -711,8 +592,6 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, VIR_FREE(port); VIR_FREE(nodeset); - VIR_FREE(adapter_type); - VIR_FREE(managed); virStorageAuthDefFree(authdef); return ret; } @@ -928,37 +807,8 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) goto error; } - if (ret->source.adapter.type == - VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) { - if (!ret->source.adapter.data.fchost.wwnn || - !ret->source.adapter.data.fchost.wwpn) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("'wwnn' and 'wwpn' must be specified for adapter " - "type 'fchost'")); - goto error; - } - - if (!virValidateWWN(ret->source.adapter.data.fchost.wwnn) || - !virValidateWWN(ret->source.adapter.data.fchost.wwpn)) - goto error; - } else if (ret->source.adapter.type == - VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) { - if (!ret->source.adapter.data.scsi_host.name && - !ret->source.adapter.data.scsi_host.has_parent) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("Either 'name' or 'parent' must be specified " - "for the 'scsi_host' adapter")); - goto error; - } - - if (ret->source.adapter.data.scsi_host.name && - ret->source.adapter.data.scsi_host.has_parent) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("Both 'name' and 'parent' cannot be specified " - "for the 'scsi_host' adapter")); - goto error; - } - } + if (virStorageAdapterParseValidate(&ret->source.adapter) < 0) + goto error; } /* If DEVICE is the only source type, then its required */ @@ -1112,49 +962,10 @@ virStoragePoolSourceFormat(virBufferPtr buf, if (options->flags & VIR_STORAGE_POOL_SOURCE_DIR) virBufferEscapeString(buf, "<dir path='%s'/>\n", src->dir); - if ((options->flags & VIR_STORAGE_POOL_SOURCE_ADAPTER)) { - if (src->adapter.type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST || - src->adapter.type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) - virBufferAsprintf(buf, "<adapter type='%s'", - virStoragePoolSourceAdapterTypeToString(src->adapter.type)); - - if (src->adapter.type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) { - virBufferEscapeString(buf, " parent='%s'", - src->adapter.data.fchost.parent); - if (src->adapter.data.fchost.managed) - virBufferAsprintf(buf, " managed='%s'", - virTristateBoolTypeToString(src->adapter.data.fchost.managed)); - virBufferEscapeString(buf, " parent_wwnn='%s'", - src->adapter.data.fchost.parent_wwnn); - virBufferEscapeString(buf, " parent_wwpn='%s'", - src->adapter.data.fchost.parent_wwpn); - virBufferEscapeString(buf, " parent_fabric_wwn='%s'", - src->adapter.data.fchost.parent_fabric_wwn); - - virBufferAsprintf(buf, " wwnn='%s' wwpn='%s'/>\n", - src->adapter.data.fchost.wwnn, - src->adapter.data.fchost.wwpn); - } else if (src->adapter.type == - VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) { - if (src->adapter.data.scsi_host.name) { - virBufferAsprintf(buf, " name='%s'/>\n", - src->adapter.data.scsi_host.name); - } else { - virPCIDeviceAddress addr; - virBufferAddLit(buf, ">\n"); - virBufferAdjustIndent(buf, 2); - virBufferAsprintf(buf, "<parentaddr unique_id='%d'>\n", - src->adapter.data.scsi_host.unique_id); - virBufferAdjustIndent(buf, 2); - addr = src->adapter.data.scsi_host.parentaddr; - ignore_value(virPCIDeviceAddressFormat(buf, addr, false)); - virBufferAdjustIndent(buf, -2); - virBufferAddLit(buf, "</parentaddr>\n"); - virBufferAdjustIndent(buf, -2); - virBufferAddLit(buf, "</adapter>\n"); - } - } - } + if ((options->flags & VIR_STORAGE_POOL_SOURCE_ADAPTER) && + (src->adapter.type == VIR_STORAGE_ADAPTER_TYPE_FC_HOST || + src->adapter.type == VIR_STORAGE_ADAPTER_TYPE_SCSI_HOST)) + virStorageAdapterFormat(buf, &src->adapter); if (options->flags & VIR_STORAGE_POOL_SOURCE_NAME) virBufferEscapeString(buf, "<name>%s</name>\n", src->name); @@ -2279,27 +2090,20 @@ virStoragePoolObjIsDuplicate(virStoragePoolObjListPtr pools, static int -getSCSIHostNumber(virStoragePoolSourceAdapter adapter, +getSCSIHostNumber(virStorageAdapterSCSIHostPtr scsi_host, unsigned int *hostnum) { int ret = -1; unsigned int num; char *name = NULL; - if (adapter.data.scsi_host.has_parent) { - virPCIDeviceAddress addr = adapter.data.scsi_host.parentaddr; - unsigned int unique_id = adapter.data.scsi_host.unique_id; - - if (!(name = virSCSIHostGetNameByParentaddr(addr.domain, - addr.bus, - addr.slot, - addr.function, - unique_id))) + if (scsi_host->has_parent) { + if (!(name = virSCSIHostGetNameByParentaddr(scsi_host))) goto cleanup; if (virSCSIHostGetNumber(name, &num) < 0) goto cleanup; } else { - if (virSCSIHostGetNumber(adapter.data.scsi_host.name, &num) < 0) + if (virSCSIHostGetNumber(scsi_host->name, &num) < 0) goto cleanup; } @@ -2330,7 +2134,7 @@ virStorageIsSameHostnum(const char *name, * matchFCHostToSCSIHost: * * @conn: Connection pointer - * @fc_adapter: fc_host adapter (either def or pool->def) + * @fchost: fc_host adapter ptr (either def or pool->def) * @scsi_hostnum: Already determined "scsi_pool" hostnum * * Returns true/false whether there is a match between the incoming @@ -2338,7 +2142,7 @@ virStorageIsSameHostnum(const char *name, */ static bool matchFCHostToSCSIHost(virConnectPtr conn, - virStoragePoolSourceAdapter fc_adapter, + virStorageAdapterFCHostPtr fchost, unsigned int scsi_hostnum) { char *name = NULL; @@ -2348,16 +2152,14 @@ matchFCHostToSCSIHost(virConnectPtr conn, /* If we have a parent defined, get its hostnum, and compare to the * scsi_hostnum. If they are the same, then we have a match */ - if (fc_adapter.data.fchost.parent && - virStorageIsSameHostnum(fc_adapter.data.fchost.parent, scsi_hostnum)) + if (fchost->parent && + virStorageIsSameHostnum(fchost->parent, scsi_hostnum)) return true; - /* If we find an fc_adapter name, then either libvirt created a vHBA + /* If we find an fc adapter name, then either libvirt created a vHBA * for this fc_host or a 'virsh nodedev-create' generated a vHBA. */ - if ((name = virVHBAGetHostByWWN(NULL, fc_adapter.data.fchost.wwnn, - fc_adapter.data.fchost.wwpn))) { - + if ((name = virVHBAGetHostByWWN(NULL, fchost->wwnn, fchost->wwpn))) { /* Get the scsi_hostN for the vHBA in order to see if it * matches our scsi_hostnum */ @@ -2372,7 +2174,7 @@ matchFCHostToSCSIHost(virConnectPtr conn, * If the parent fc_hostnum is the same as the scsi_hostnum, we * have a match. */ - if (conn && !fc_adapter.data.fchost.parent) { + if (conn && !fchost->parent) { if (virAsprintf(&scsi_host_name, "scsi_%s", name) < 0) { VIR_FREE(name); return false; @@ -2404,25 +2206,21 @@ matchFCHostToSCSIHost(virConnectPtr conn, return false; } + static bool -matchSCSIAdapterParent(virStoragePoolObjPtr pool, - virStoragePoolDefPtr def) +matchSCSIAdapterParent(virStorageAdapterSCSIHostPtr pool_scsi_host, + virStorageAdapterSCSIHostPtr def_scsi_host) { - virPCIDeviceAddressPtr pooladdr = - &pool->def->source.adapter.data.scsi_host.parentaddr; - virPCIDeviceAddressPtr defaddr = - &def->source.adapter.data.scsi_host.parentaddr; - int pool_unique_id = - pool->def->source.adapter.data.scsi_host.unique_id; - int def_unique_id = - def->source.adapter.data.scsi_host.unique_id; + virPCIDeviceAddressPtr pooladdr = &pool_scsi_host->parentaddr; + virPCIDeviceAddressPtr defaddr = &def_scsi_host->parentaddr; + if (pooladdr->domain == defaddr->domain && pooladdr->bus == defaddr->bus && pooladdr->slot == defaddr->slot && pooladdr->function == defaddr->function && - pool_unique_id == def_unique_id) { + pool_scsi_host->unique_id == def_scsi_host->unique_id) return true; - } + return false; } @@ -2465,6 +2263,8 @@ virStoragePoolSourceFindDuplicate(virConnectPtr conn, int ret = 1; virStoragePoolObjPtr pool = NULL; virStoragePoolObjPtr matchpool = NULL; + virStorageAdapterPtr pool_adapter; + virStorageAdapterPtr def_adapter; /* Check the pool list for duplicate underlying storage */ for (i = 0; i < pools->count; i++) { @@ -2500,62 +2300,70 @@ virStoragePoolSourceFindDuplicate(virConnectPtr conn, break; case VIR_STORAGE_POOL_SCSI: - if (pool->def->source.adapter.type == - VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST && - def->source.adapter.type == - VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) { - if (STREQ(pool->def->source.adapter.data.fchost.wwnn, - def->source.adapter.data.fchost.wwnn) && - STREQ(pool->def->source.adapter.data.fchost.wwpn, - def->source.adapter.data.fchost.wwpn)) + pool_adapter = &pool->def->source.adapter; + def_adapter = &def->source.adapter; + if (pool_adapter->type == VIR_STORAGE_ADAPTER_TYPE_FC_HOST && + def_adapter->type == VIR_STORAGE_ADAPTER_TYPE_FC_HOST) { + virStorageAdapterFCHostPtr pool_fchost = + &pool_adapter->data.fchost; + virStorageAdapterFCHostPtr def_fchost = + &def_adapter->data.fchost; + if (STREQ(pool_fchost->wwnn, def_fchost->wwnn) && + STREQ(pool_fchost->wwpn, def_fchost->wwpn)) matchpool = pool; - } else if (pool->def->source.adapter.type == - VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST && - def->source.adapter.type == - VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) { + } else if (pool_adapter->type == + VIR_STORAGE_ADAPTER_TYPE_SCSI_HOST && + def_adapter->type == + VIR_STORAGE_ADAPTER_TYPE_SCSI_HOST) { + virStorageAdapterSCSIHostPtr pool_scsi_host = + &pool_adapter->data.scsi_host; + virStorageAdapterSCSIHostPtr def_scsi_host = + &def_adapter->data.scsi_host; unsigned int pool_hostnum, def_hostnum; - if (pool->def->source.adapter.data.scsi_host.has_parent && - def->source.adapter.data.scsi_host.has_parent && - matchSCSIAdapterParent(pool, def)) { + if (pool_scsi_host->has_parent && + def_scsi_host->has_parent && + matchSCSIAdapterParent(pool_scsi_host, def_scsi_host)) { matchpool = pool; break; } - if (getSCSIHostNumber(pool->def->source.adapter, - &pool_hostnum) < 0 || - getSCSIHostNumber(def->source.adapter, &def_hostnum) < 0) + if (getSCSIHostNumber(pool_scsi_host, &pool_hostnum) < 0 || + getSCSIHostNumber(def_scsi_host, &def_hostnum) < 0) break; if (pool_hostnum == def_hostnum) matchpool = pool; - } else if (pool->def->source.adapter.type == - VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST && - def->source.adapter.type == - VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) { + } else if (pool_adapter->type == + VIR_STORAGE_ADAPTER_TYPE_FC_HOST && + def_adapter->type == + VIR_STORAGE_ADAPTER_TYPE_SCSI_HOST) { + virStorageAdapterFCHostPtr pool_fchost = + &pool_adapter->data.fchost; + virStorageAdapterSCSIHostPtr def_scsi_host = + &def_adapter->data.scsi_host; unsigned int scsi_hostnum; /* Get the scsi_hostN for the scsi_host source adapter def */ - if (getSCSIHostNumber(def->source.adapter, - &scsi_hostnum) < 0) + if (getSCSIHostNumber(def_scsi_host, &scsi_hostnum) < 0) break; - if (matchFCHostToSCSIHost(conn, pool->def->source.adapter, - scsi_hostnum)) { + if (matchFCHostToSCSIHost(conn, pool_fchost, scsi_hostnum)) { matchpool = pool; break; } - } else if (pool->def->source.adapter.type == - VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST && - def->source.adapter.type == - VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) { + } else if (pool_adapter->type == + VIR_STORAGE_ADAPTER_TYPE_SCSI_HOST && + def_adapter->type == + VIR_STORAGE_ADAPTER_TYPE_FC_HOST) { + virStorageAdapterSCSIHostPtr pool_scsi_host = + &pool_adapter->data.scsi_host; unsigned int scsi_hostnum; - if (getSCSIHostNumber(pool->def->source.adapter, - &scsi_hostnum) < 0) + if (getSCSIHostNumber(pool_scsi_host, &scsi_hostnum) < 0) break; - if (matchFCHostToSCSIHost(conn, def->source.adapter, + if (matchFCHostToSCSIHost(conn, &def_adapter->data.fchost, scsi_hostnum)) { matchpool = pool; break; diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index 1723afc..67813b0 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -26,6 +26,7 @@ # include "internal.h" # include "virstorageencryption.h" +# include "virstoragedevice.h" # include "virstoragefile.h" # include "virbitmap.h" # include "virthread.h" @@ -170,38 +171,6 @@ struct _virStoragePoolSourceDevice { } geometry; }; -typedef enum { - VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_DEFAULT = 0, - VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST, - VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST, - - VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_LAST, -} virStoragePoolSourceAdapterType; -VIR_ENUM_DECL(virStoragePoolSourceAdapter) - -typedef struct _virStoragePoolSourceAdapter virStoragePoolSourceAdapter; -typedef virStoragePoolSourceAdapter *virStoragePoolSourceAdapterPtr; -struct _virStoragePoolSourceAdapter { - int type; /* virStoragePoolSourceAdapterType */ - - union { - struct { - char *name; - virPCIDeviceAddress parentaddr; /* host address */ - int unique_id; - bool has_parent; - } scsi_host; - struct { - char *parent; - char *parent_wwnn; - char *parent_wwpn; - char *parent_fabric_wwn; - char *wwnn; - char *wwpn; - int managed; /* enum virTristateSwitch */ - } fchost; - } data; -}; typedef struct _virStoragePoolSource virStoragePoolSource; typedef virStoragePoolSource *virStoragePoolSourcePtr; @@ -218,7 +187,7 @@ struct _virStoragePoolSource { char *dir; /* Or an adapter */ - virStoragePoolSourceAdapter adapter; + virStorageAdapter adapter; /* Or a name */ char *name; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index e6ccd69..b88e700 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -902,8 +902,6 @@ virStoragePoolObjSaveDef; virStoragePoolObjUnlock; virStoragePoolSaveConfig; virStoragePoolSaveState; -virStoragePoolSourceAdapterTypeFromString; -virStoragePoolSourceAdapterTypeToString; virStoragePoolSourceClear; virStoragePoolSourceDeviceClear; virStoragePoolSourceFindDuplicate; @@ -2435,6 +2433,20 @@ virSocketAddrSetIPv6Addr; virSocketAddrSetIPv6AddrNetOrder; virSocketAddrSetPort; + +# util/virstoragedevice.h +virStorageAdapterClear; +virStorageAdapterFormat; +virStorageAdapterParseValidate; +virStorageAdapterParseXML; +virStorageAdapterTypeFromString; +virStorageAdapterTypeToString; +virStorageAdapterVHBAClear; +virStorageAdapterVHBAFormat; +virStorageAdapterVHBAParseValidate; +virStorageAdapterVHBAParseXML; + + # util/virstorageencryption.h virStorageEncryptionFormat; virStorageEncryptionFree; diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 4bab651..d32456c 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -46,6 +46,7 @@ #include "libxl_conf.h" #include "libxl_utils.h" #include "virstoragefile.h" +#include "virstoragedevice.h" #include "secret_util.h" diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index 7a5df3f..39fa026 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -2467,8 +2467,7 @@ phypBuildStoragePool(virConnectPtr conn, virStoragePoolDefPtr def) int exit_status = 0; virBuffer buf = VIR_BUFFER_INITIALIZER; - if (source.adapter.type != - VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) { + if (source.adapter.type != VIR_STORAGE_ADAPTER_TYPE_SCSI_HOST) { virReportError(VIR_ERR_XML_ERROR, "%s", _("Only 'scsi_host' adapter is supported")); goto cleanup; diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index cd05741..afc3c89 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -176,33 +176,30 @@ virStoragePoolFCRefreshThread(void *opaque) } static char * -getAdapterName(virStoragePoolSourceAdapter adapter) +getAdapterName(virStorageAdapterPtr adapter) { char *name = NULL; char *parentaddr = NULL; + virStorageAdapterSCSIHostPtr scsi_host; + virStorageAdapterFCHostPtr fchost; - if (adapter.type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) { - if (adapter.data.scsi_host.has_parent) { - virPCIDeviceAddress addr = adapter.data.scsi_host.parentaddr; - unsigned int unique_id = adapter.data.scsi_host.unique_id; + if (adapter->type == VIR_STORAGE_ADAPTER_TYPE_SCSI_HOST) { + scsi_host = &adapter->data.scsi_host; - if (!(name = virSCSIHostGetNameByParentaddr(addr.domain, - addr.bus, - addr.slot, - addr.function, - unique_id))) + if (scsi_host->has_parent) { + if (!(name = virSCSIHostGetNameByParentaddr(scsi_host))) goto cleanup; } else { - ignore_value(VIR_STRDUP(name, adapter.data.scsi_host.name)); + ignore_value(VIR_STRDUP(name, scsi_host->name)); } - } else if (adapter.type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) { - if (!(name = virVHBAGetHostByWWN(NULL, - adapter.data.fchost.wwnn, - adapter.data.fchost.wwpn))) { + } + + if (adapter->type == VIR_STORAGE_ADAPTER_TYPE_FC_HOST) { + fchost = &adapter->data.fchost; + if (!(name = virVHBAGetHostByWWN(NULL, fchost->wwnn, fchost->wwpn))) { virReportError(VIR_ERR_XML_ERROR, _("Failed to find SCSI host with wwnn='%s', " - "wwpn='%s'"), adapter.data.fchost.wwnn, - adapter.data.fchost.wwpn); + "wwpn='%s'"), fchost->wwnn, fchost->wwpn); } } @@ -254,10 +251,10 @@ checkParent(virConnectPtr conn, static int createVport(virConnectPtr conn, - virStoragePoolObjPtr pool) + virStoragePoolDefPtr def, + const char *configFile, + virStorageAdapterFCHostPtr fchost) { - const char *configFile = pool->configFile; - virStoragePoolSourceAdapterPtr adapter = &pool->def->source.adapter; unsigned int parent_host; char *name = NULL; char *parent_hoststr = NULL; @@ -266,45 +263,37 @@ createVport(virConnectPtr conn, virThread thread; int ret = -1; - if (adapter->type != VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) - return 0; - VIR_DEBUG("conn=%p, configFile='%s' parent='%s', wwnn='%s' wwpn='%s'", - conn, NULLSTR(configFile), NULLSTR(adapter->data.fchost.parent), - adapter->data.fchost.wwnn, adapter->data.fchost.wwpn); + conn, NULLSTR(configFile), NULLSTR(fchost->parent), + fchost->wwnn, fchost->wwpn); /* If we find an existing HBA/vHBA within the fc_host sysfs * using the wwnn/wwpn, then a nodedev is already created for * this pool and we don't have to create the vHBA */ - if ((name = virVHBAGetHostByWWN(NULL, adapter->data.fchost.wwnn, - adapter->data.fchost.wwpn))) { + if ((name = virVHBAGetHostByWWN(NULL, fchost->wwnn, fchost->wwpn))) { /* If a parent was provided, let's make sure the 'name' we've * retrieved has the same parent */ - if (adapter->data.fchost.parent && - checkParent(conn, name, adapter->data.fchost.parent)) + if (fchost->parent && checkParent(conn, name, fchost->parent)) ret = 0; goto cleanup; } - if (adapter->data.fchost.parent) { - if (VIR_STRDUP(parent_hoststr, adapter->data.fchost.parent) < 0) + if (fchost->parent) { + if (VIR_STRDUP(parent_hoststr, fchost->parent) < 0) goto cleanup; - } else if (adapter->data.fchost.parent_wwnn && - adapter->data.fchost.parent_wwpn) { - if (!(parent_hoststr = - virVHBAGetHostByWWN(NULL, adapter->data.fchost.parent_wwnn, - adapter->data.fchost.parent_wwpn))) { + } else if (fchost->parent_wwnn && fchost->parent_wwpn) { + if (!(parent_hoststr = virVHBAGetHostByWWN(NULL, fchost->parent_wwnn, + fchost->parent_wwpn))) { virReportError(VIR_ERR_XML_ERROR, "%s", _("cannot find parent using provided wwnn/wwpn")); goto cleanup; } - } else if (adapter->data.fchost.parent_fabric_wwn) { + } else if (fchost->parent_fabric_wwn) { if (!(parent_hoststr = - virVHBAGetHostByFabricWWN(NULL, - adapter->data.fchost.parent_fabric_wwn))) { + virVHBAGetHostByFabricWWN(NULL, fchost->parent_fabric_wwn))) { virReportError(VIR_ERR_XML_ERROR, "%s", _("cannot find parent using provided fabric_wwn")); goto cleanup; @@ -323,8 +312,8 @@ createVport(virConnectPtr conn, goto cleanup; /* NOTE: - * We do not save the parent_hoststr in adapter->data.fchost.parent - * since we could be writing out the 'def' to the saved XML config. + * We do not save the parent_hoststr in fchost->parent since + * we could be writing out the 'def' to the saved XML config. * If we wrote out the name in the XML, then future starts would * always use the same parent rather than finding the "best available" * parent. Besides we have a way to determine the parent based on @@ -342,16 +331,16 @@ createVport(virConnectPtr conn, * restart, we need to save the persistent configuration. So if not * already defined as YES, then force the issue. */ - if (adapter->data.fchost.managed != VIR_TRISTATE_BOOL_YES) { - adapter->data.fchost.managed = VIR_TRISTATE_BOOL_YES; + if (fchost->managed != VIR_TRISTATE_BOOL_YES) { + fchost->managed = VIR_TRISTATE_BOOL_YES; if (configFile) { - if (virStoragePoolSaveConfig(configFile, pool->def) < 0) + if (virStoragePoolSaveConfig(configFile, def) < 0) goto cleanup; } } - if (virVHBAManageVport(parent_host, adapter->data.fchost.wwpn, - adapter->data.fchost.wwnn, VPORT_CREATE) < 0) + if (virVHBAManageVport(parent_host, fchost->wwpn, fchost->wwnn, + VPORT_CREATE) < 0) goto cleanup; virFileWaitForDevices(); @@ -361,10 +350,9 @@ createVport(virConnectPtr conn, * retry logic set to true. If the thread isn't created, then no big * deal since it's still possible to refresh the pool later. */ - if ((name = virVHBAGetHostByWWN(NULL, adapter->data.fchost.wwnn, - adapter->data.fchost.wwpn))) { + if ((name = virVHBAGetHostByWWN(NULL, fchost->wwnn, fchost->wwpn))) { if (VIR_ALLOC(cbdata) == 0) { - memcpy(cbdata->pool_uuid, pool->def->uuid, VIR_UUID_BUFLEN); + memcpy(cbdata->pool_uuid, def->uuid, VIR_UUID_BUFLEN); VIR_STEAL_PTR(cbdata->fchost_name, name); if (virThreadCreate(&thread, false, virStoragePoolFCRefreshThread, @@ -384,9 +372,10 @@ createVport(virConnectPtr conn, return ret; } + static int deleteVport(virConnectPtr conn, - virStoragePoolSourceAdapter adapter) + virStorageAdapterFCHostPtr fchost) { unsigned int parent_host; char *name = NULL; @@ -394,25 +383,19 @@ deleteVport(virConnectPtr conn, char *vhba_parent = NULL; int ret = -1; - if (adapter.type != VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) - return 0; - VIR_DEBUG("conn=%p parent='%s', managed='%d' wwnn='%s' wwpn='%s'", - conn, NULLSTR(adapter.data.fchost.parent), - adapter.data.fchost.managed, - adapter.data.fchost.wwnn, - adapter.data.fchost.wwpn); + conn, NULLSTR(fchost->parent), fchost->managed, + fchost->wwnn, fchost->wwpn); /* If we're not managing the deletion of the vHBA, then just return */ - if (adapter.data.fchost.managed != VIR_TRISTATE_BOOL_YES) + if (fchost->managed != VIR_TRISTATE_BOOL_YES) return 0; /* Find our vHBA by searching the fc_host sysfs tree for our wwnn/wwpn */ - if (!(name = virVHBAGetHostByWWN(NULL, adapter.data.fchost.wwnn, - adapter.data.fchost.wwpn))) { + if (!(name = virVHBAGetHostByWWN(NULL, fchost->wwnn, fchost->wwpn))) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to find fc_host for wwnn='%s' and wwpn='%s'"), - adapter.data.fchost.wwnn, adapter.data.fchost.wwpn); + fchost->wwnn, fchost->wwpn); goto cleanup; } @@ -420,8 +403,8 @@ deleteVport(virConnectPtr conn, * get the parent_host value; otherwise, we have to determine * the parent scsi_host which we did not save at startup time */ - if (adapter.data.fchost.parent) { - if (virSCSIHostGetNumber(adapter.data.fchost.parent, &parent_host) < 0) + if (fchost->parent) { + if (virSCSIHostGetNumber(fchost->parent, &parent_host) < 0) goto cleanup; } else { if (virAsprintf(&scsi_host_name, "scsi_%s", name) < 0) @@ -434,8 +417,8 @@ deleteVport(virConnectPtr conn, goto cleanup; } - if (virVHBAManageVport(parent_host, adapter.data.fchost.wwpn, - adapter.data.fchost.wwnn, VPORT_DELETE) < 0) + if (virVHBAManageVport(parent_host, fchost->wwpn, fchost->wwnn, + VPORT_DELETE) < 0) goto cleanup; ret = 0; @@ -458,13 +441,13 @@ virStorageBackendSCSICheckPool(virStoragePoolObjPtr pool, *isActive = false; - if (!(name = getAdapterName(pool->def->source.adapter))) { + if (!(name = getAdapterName(&pool->def->source.adapter))) { /* It's normal for the pool with "fc_host" type source * adapter fails to get the adapter name, since the vHBA * the adapter based on might be not created yet. */ if (pool->def->source.adapter.type == - VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) { + VIR_STORAGE_ADAPTER_TYPE_FC_HOST) { virResetLastError(); return 0; } else { @@ -498,7 +481,7 @@ virStorageBackendSCSIRefreshPool(virConnectPtr conn ATTRIBUTE_UNUSED, pool->def->allocation = pool->def->capacity = pool->def->available = 0; - if (!(name = getAdapterName(pool->def->source.adapter))) + if (!(name = getAdapterName(&pool->def->source.adapter))) return -1; if (virSCSIHostGetNumber(name, &host) < 0) @@ -522,15 +505,21 @@ static int virStorageBackendSCSIStartPool(virConnectPtr conn, virStoragePoolObjPtr pool) { - return createVport(conn, pool); + if (pool->def->source.adapter.type == VIR_STORAGE_ADAPTER_TYPE_FC_HOST) + return createVport(conn, pool->def, pool->configFile, + &pool->def->source.adapter.data.fchost); + + return 0; } static int virStorageBackendSCSIStopPool(virConnectPtr conn, virStoragePoolObjPtr pool) { - virStoragePoolSourceAdapter adapter = pool->def->source.adapter; - return deleteVport(conn, adapter); + if (pool->def->source.adapter.type == VIR_STORAGE_ADAPTER_TYPE_FC_HOST) + return deleteVport(conn, &pool->def->source.adapter.data.fchost); + + return 0; } virStorageBackend virStorageBackendSCSI = { diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 4dff0f1..5733a20 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -4422,7 +4422,7 @@ testStoragePoolCreateXML(virConnectPtr conn, def = NULL; if (pool->def->source.adapter.type == - VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) { + VIR_STORAGE_ADAPTER_TYPE_FC_HOST) { /* In the real code, we'd call virVHBAManageVport followed by * find_new_device, but we cannot do that here since we're not * mocking udev. The mock routine will copy an existing vHBA and @@ -4645,7 +4645,7 @@ testStoragePoolDestroy(virStoragePoolPtr pool) privpool->active = 0; if (privpool->def->source.adapter.type == - VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) { + VIR_STORAGE_ADAPTER_TYPE_FC_HOST) { if (testDestroyVport(privconn, privpool->def->source.adapter.data.fchost.wwnn, privpool->def->source.adapter.data.fchost.wwpn) < 0) diff --git a/src/util/virscsihost.c b/src/util/virscsihost.c index eea0474..dffc958 100644 --- a/src/util/virscsihost.c +++ b/src/util/virscsihost.c @@ -218,36 +218,30 @@ virSCSIHostGetNumber(const char *adapter_name, } /* virSCSIHostGetNameByParentaddr: - * @domain: The domain from the scsi_host parentaddr - * @bus: The bus from the scsi_host parentaddr - * @slot: The slot from the scsi_host parentaddr - * @function: The function from the scsi_host parentaddr - * @unique_id: The unique id value for parentaddr + * @scsi_host: The scsi_host adapter pointer. * * Generate a parentaddr and find the scsi_host host# for - * the provided parentaddr PCI address fields. + * the provided scsi_host. * * Returns the "host#" string which must be free'd by * the caller or NULL on error */ char * -virSCSIHostGetNameByParentaddr(unsigned int domain, - unsigned int bus, - unsigned int slot, - unsigned int function, - unsigned int unique_id) +virSCSIHostGetNameByParentaddr(virStorageAdapterSCSIHostPtr scsi_host) { + virPCIDeviceAddressPtr addr = &scsi_host->parentaddr; char *name = NULL; char *parentaddr = NULL; if (virAsprintf(&parentaddr, "%04x:%02x:%02x.%01x", - domain, bus, slot, function) < 0) + addr->domain, addr->bus, addr->slot, addr->function) < 0) goto cleanup; - if (!(name = virSCSIHostFindByPCI(NULL, parentaddr, unique_id))) { + if (!(name = virSCSIHostFindByPCI(NULL, parentaddr, + scsi_host->unique_id))) { virReportError(VIR_ERR_XML_ERROR, _("Failed to find scsi_host using PCI '%s' " "and unique_id='%u'"), - parentaddr, unique_id); + parentaddr, scsi_host->unique_id); goto cleanup; } @@ -284,11 +278,7 @@ virSCSIHostGetNumber(const char *adapter_name ATTRIBUTE_UNUSED, } char * -virSCSIHostGetNameByParentaddr(unsigned int domain ATTRIBUTE_UNUSED, - unsigned int bus ATTRIBUTE_UNUSED, - unsigned int slot ATTRIBUTE_UNUSED, - unsigned int function ATTRIBUTE_UNUSED, - unsigned int unique_id ATTRIBUTE_UNUSED) +virSCSIHostGetNameByParentaddr(virStorageAdapterSCSIHostPtr scsi_host) { virReportSystemError(ENOSYS, "%s", _("Not supported on this platform")); return NULL; diff --git a/src/util/virscsihost.h b/src/util/virscsihost.h index c35ccb9..e0eddaf 100644 --- a/src/util/virscsihost.h +++ b/src/util/virscsihost.h @@ -21,6 +21,8 @@ # include "internal.h" +# include "virstoragedevice.h" + int virSCSIHostGetUniqueId(const char *sysfs_prefix, int host); char *virSCSIHostFindByPCI(const char *sysfs_prefix, @@ -31,10 +33,6 @@ int virSCSIHostGetNumber(const char *adapter_name, unsigned int *result) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); -char *virSCSIHostGetNameByParentaddr(unsigned int domain, - unsigned int bus, - unsigned int slot, - unsigned int function, - unsigned int unique_id); +char *virSCSIHostGetNameByParentaddr(virStorageAdapterSCSIHostPtr scsi_host); #endif /* __VIR_SCSI_HOST_H__ */ diff --git a/src/util/virstoragedevice.c b/src/util/virstoragedevice.c new file mode 100644 index 0000000..0d9db34 --- /dev/null +++ b/src/util/virstoragedevice.c @@ -0,0 +1,292 @@ +/* + * virstoragedevice.c: utility functions to share storage device mgmt + * between storage pools and domains + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + */ + +#include <config.h> + +#include "device_conf.h" + +#include "viralloc.h" +#include "virerror.h" +#include "virlog.h" +#include "virstoragedevice.h" +#include "virstring.h" +#include "virutil.h" +#include "virxml.h" + +#define VIR_FROM_THIS VIR_FROM_STORAGE + +VIR_LOG_INIT("util.virstoragedevice"); + +VIR_ENUM_IMPL(virStorageAdapter, + VIR_STORAGE_ADAPTER_TYPE_LAST, + "default", "scsi_host", "fc_host") + + +int +virStorageAdapterVHBAParseXML(xmlNodePtr node, + virStorageAdapterFCHostPtr fchost) +{ + char *managed = NULL; + + fchost->parent = virXMLPropString(node, "parent"); + + if ((managed = virXMLPropString(node, "managed"))) { + if ((fchost->managed = + virTristateBoolTypeFromString(managed)) < 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown fc_host managed setting '%s'"), + managed); + VIR_FREE(managed); + return -1; + } + } + + fchost->parent_wwnn = virXMLPropString(node, "parent_wwnn"); + fchost->parent_wwpn = virXMLPropString(node, "parent_wwpn"); + fchost->parent_fabric_wwn = virXMLPropString(node, "parent_fabric_wwn"); + fchost->wwpn = virXMLPropString(node, "wwpn"); + fchost->wwnn = virXMLPropString(node, "wwnn"); + + VIR_FREE(managed); + return 0; +} + + +int +virStorageAdapterParseXML(virStorageAdapterPtr adapter, + xmlNodePtr node, + xmlXPathContextPtr ctxt) +{ + int ret = -1; + xmlNodePtr relnode = ctxt->node; + char *adapter_type = NULL; + virStorageAdapterSCSIHostPtr scsi_host; + + ctxt->node = node; + + if ((adapter_type = virXMLPropString(node, "type"))) { + if ((adapter->type = + virStorageAdapterTypeFromString(adapter_type)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Unknown pool adapter type '%s'"), + adapter_type); + goto cleanup; + } + + if (adapter->type == VIR_STORAGE_ADAPTER_TYPE_FC_HOST) { + if (virStorageAdapterVHBAParseXML(node, &adapter->data.fchost) < 0) + goto cleanup; + } + + if (adapter->type == VIR_STORAGE_ADAPTER_TYPE_SCSI_HOST) { + scsi_host = &adapter->data.scsi_host; + + scsi_host->name = virXMLPropString(node, "name"); + if (virXPathNode("./parentaddr", ctxt)) { + xmlNodePtr addrnode = virXPathNode("./parentaddr/address", + ctxt); + virPCIDeviceAddressPtr addr = &scsi_host->parentaddr; + if (!addrnode) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Missing scsi_host PCI address element")); + goto cleanup; + } + scsi_host->has_parent = true; + if (virPCIDeviceAddressParseXML(addrnode, addr) < 0) + goto cleanup; + if ((virXPathInt("string(./parentaddr/@unique_id)", + ctxt, + &scsi_host->unique_id) < 0) || + (scsi_host->unique_id < 0)) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Missing or invalid scsi adapter " + "'unique_id' value")); + goto cleanup; + } + } + } + } else { + char *wwnn = virXMLPropString(node, "wwnn"); + char *wwpn = virXMLPropString(node, "wwpn"); + char *parent = virXMLPropString(node, "parent"); + + /* "type" was not specified in the XML, so we must verify that + * "wwnn", "wwpn", "parent", or "parentaddr" are also not in the + * XML. If any are found, then we cannot just use "name" alone". + */ + if (wwnn || wwpn || parent) { + VIR_FREE(wwnn); + VIR_FREE(wwpn); + VIR_FREE(parent); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Use of 'wwnn', 'wwpn', and 'parent' attributes " + "requires use of the adapter 'type'")); + goto cleanup; + } + + if (virXPathNode("./parentaddr", ctxt)) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Use of 'parent' element requires use " + "of the adapter 'type'")); + goto cleanup; + } + + /* To keep back-compat, 'type' is not required to specify + * for scsi_host adapter. + */ + if ((adapter->data.scsi_host.name = virXMLPropString(node, "name"))) + adapter->type = VIR_STORAGE_ADAPTER_TYPE_SCSI_HOST; + } + + ret = 0; + + cleanup: + ctxt->node = relnode; + VIR_FREE(adapter_type); + return ret; +} + + +void +virStorageAdapterVHBAFormat(virBufferPtr buf, + virStorageAdapterFCHostPtr fchost) +{ + virBufferEscapeString(buf, " parent='%s'", fchost->parent); + virBufferEscapeString(buf, " parent_wwnn='%s'", fchost->parent_wwnn); + virBufferEscapeString(buf, " parent_wwpn='%s'", fchost->parent_wwpn); + virBufferEscapeString(buf, " parent_fabric_wwn='%s'", + fchost->parent_fabric_wwn); + if (fchost->managed != VIR_TRISTATE_BOOL_ABSENT) + virBufferAsprintf(buf, " managed='%s'", + virTristateBoolTypeToString(fchost->managed)); + + virBufferAsprintf(buf, " wwnn='%s' wwpn='%s'/>\n", + fchost->wwnn, fchost->wwpn); +} + + +void +virStorageAdapterFormat(virBufferPtr buf, + virStorageAdapterPtr adapter) +{ + virBufferAsprintf(buf, "<adapter type='%s'", + virStorageAdapterTypeToString(adapter->type)); + + if (adapter->type == VIR_STORAGE_ADAPTER_TYPE_FC_HOST) + virStorageAdapterVHBAFormat(buf, &adapter->data.fchost); + + if (adapter->type == VIR_STORAGE_ADAPTER_TYPE_SCSI_HOST) { + virStorageAdapterSCSIHostPtr scsi_host = &adapter->data.scsi_host; + + if (scsi_host->name) { + virBufferAsprintf(buf, " name='%s'/>\n", scsi_host->name); + } else { + virPCIDeviceAddress addr; + virBufferAddLit(buf, ">\n"); + virBufferAdjustIndent(buf, 2); + virBufferAsprintf(buf, "<parentaddr unique_id='%d'>\n", + scsi_host->unique_id); + virBufferAdjustIndent(buf, 2); + addr = scsi_host->parentaddr; + ignore_value(virPCIDeviceAddressFormat(buf, addr, false)); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</parentaddr>\n"); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</adapter>\n"); + } + } +} + + +void +virStorageAdapterVHBAClear(virStorageAdapterFCHostPtr fchost) +{ + VIR_FREE(fchost->parent); + VIR_FREE(fchost->parent_wwnn); + VIR_FREE(fchost->parent_wwpn); + VIR_FREE(fchost->parent_fabric_wwn); + VIR_FREE(fchost->wwnn); + VIR_FREE(fchost->wwpn); +} + + +void +virStorageAdapterClear(virStorageAdapterPtr adapter) +{ + if (adapter->type == VIR_STORAGE_ADAPTER_TYPE_FC_HOST) + virStorageAdapterVHBAClear(&adapter->data.fchost); + + if (adapter->type == VIR_STORAGE_ADAPTER_TYPE_SCSI_HOST) + VIR_FREE(adapter->data.scsi_host.name); +} + + +int +virStorageAdapterVHBAParseValidate(virStorageAdapterFCHostPtr fchost) +{ + if (!fchost->wwnn || !fchost->wwpn) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("'wwnn' and 'wwpn' must be specified for " + "a VHBA adapter")); + return -1; + } + + if (!virValidateWWN(fchost->wwnn) || !virValidateWWN(fchost->wwpn)) + return -1; + + if (fchost->parent_wwnn && !virValidateWWN(fchost->parent_wwnn)) + return -1; + + if (fchost->parent_wwpn && !virValidateWWN(fchost->parent_wwpn)) + return -1; + + if (fchost->parent_fabric_wwn && + !virValidateWWN(fchost->parent_fabric_wwn)) + return -1; + + return 0; +} + + +int +virStorageAdapterParseValidate(virStorageAdapterPtr adapter) +{ + if (adapter->type == VIR_STORAGE_ADAPTER_TYPE_FC_HOST) + return virStorageAdapterVHBAParseValidate(&adapter->data.fchost); + + if (adapter->type == VIR_STORAGE_ADAPTER_TYPE_SCSI_HOST) { + virStorageAdapterSCSIHostPtr scsi_host = &adapter->data.scsi_host; + + if (!scsi_host->name && !scsi_host->has_parent) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Either 'name' or 'parent' must be specified " + "for the 'scsi_host' adapter")); + return -1; + } + + if (scsi_host->name && scsi_host->has_parent) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Both 'name' and 'parent' cannot be specified " + "for the 'scsi_host' adapter")); + return -1; + } + } + + return 0; +} diff --git a/src/util/virstoragedevice.h b/src/util/virstoragedevice.h new file mode 100644 index 0000000..c8459c7 --- /dev/null +++ b/src/util/virstoragedevice.h @@ -0,0 +1,89 @@ +/* + * virstoragedevice.h: utility functions to share storage device mgmt + * between storage pools and domains + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + */ + +#ifndef __VIR_STORAGE_DEVICE_H__ +# define __VIR_STORAGE_DEVICE_H__ + +# include "virpci.h" +# include "virxml.h" + + +typedef enum { + VIR_STORAGE_ADAPTER_TYPE_DEFAULT = 0, + VIR_STORAGE_ADAPTER_TYPE_SCSI_HOST, + VIR_STORAGE_ADAPTER_TYPE_FC_HOST, + + VIR_STORAGE_ADAPTER_TYPE_LAST, +} virStorageAdapterType; +VIR_ENUM_DECL(virStorageAdapter) + +typedef struct _virStorageAdapterSCSIHost virStorageAdapterSCSIHost; +typedef virStorageAdapterSCSIHost *virStorageAdapterSCSIHostPtr; +struct _virStorageAdapterSCSIHost { + char *name; + virPCIDeviceAddress parentaddr; /* host address */ + int unique_id; + bool has_parent; +}; + +typedef struct _virStorageAdapterFCHost virStorageAdapterFCHost; +typedef virStorageAdapterFCHost *virStorageAdapterFCHostPtr; +struct _virStorageAdapterFCHost { + char *parent; + char *parent_wwnn; + char *parent_wwpn; + char *parent_fabric_wwn; + char *wwnn; + char *wwpn; + int managed; /* enum virTristateSwitch */ +}; + +typedef struct _virStorageAdapter virStorageAdapter; +typedef virStorageAdapter *virStorageAdapterPtr; +struct _virStorageAdapter { + int type; /* virStorageAdapterType */ + + union { + virStorageAdapterSCSIHost scsi_host; + virStorageAdapterFCHost fchost; + } data; +}; + + +int virStorageAdapterVHBAParseXML(xmlNodePtr node, + virStorageAdapterFCHostPtr fchost); + +int virStorageAdapterParseXML(virStorageAdapterPtr adapter, + xmlNodePtr node, + xmlXPathContextPtr ctxt); + +void virStorageAdapterVHBAFormat(virBufferPtr buf, + virStorageAdapterFCHostPtr fchost); + +void virStorageAdapterFormat(virBufferPtr buf, virStorageAdapterPtr adapter); + +void virStorageAdapterVHBAClear(virStorageAdapterFCHostPtr fchost); + +void virStorageAdapterClear(virStorageAdapterPtr adapter); + +int virStorageAdapterVHBAParseValidate(virStorageAdapterFCHostPtr fchost); + +int virStorageAdapterParseValidate(virStorageAdapterPtr adapter); + +#endif /* __VIR_STORAGE_DEVICE_H__ */ -- 2.9.3
On 20.02.2017 14:18, John Ferlan wrote:
Introduce src/util/virstoragedevice.{c,h}. Rather than have virstoragefile be a repository for more things - create a new module to share the adapter definitions between the storage pool and eventually the domain. The devices will need device address functions (no need to pollute more code for that).
Move the virStoragePoolSourceAdapter from storage_conf into the new module and parcel out the the structure a bit more into 'fchost' and 'scsi_host' specific pieces creating virStoragePoolSourceAdapterSCSIHost and virStoragePoolSourceAdapterFCHost structures for easier access.
Modify code in the various places which formerly used the pool structure to use the new model. This includes some changes that will use the Ptr rather than just the struct (try to shorten the number of times one has to type adapter.data.fchost or adapter.data.scsi_host as well as [pool->]def->source.adapter.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- po/POTFILES.in | 1 + src/Makefile.am | 1 + src/conf/storage_conf.c | 338 ++++++++----------------------------- src/conf/storage_conf.h | 35 +--- src/libvirt_private.syms | 16 +- src/libxl/libxl_conf.c | 1 + src/phyp/phyp_driver.c | 3 +- src/storage/storage_backend_scsi.c | 131 +++++++------- src/test/test_driver.c | 4 +- src/util/virscsihost.c | 28 +-- src/util/virscsihost.h | 8 +- src/util/virstoragedevice.c | 292 ++++++++++++++++++++++++++++++++ src/util/virstoragedevice.h | 89 ++++++++++ 13 files changed, 548 insertions(+), 399 deletions(-) create mode 100644 src/util/virstoragedevice.c create mode 100644 src/util/virstoragedevice.h
diff --git a/src/util/virstoragedevice.c b/src/util/virstoragedevice.c new file mode 100644 index 0000000..0d9db34 --- /dev/null +++ b/src/util/virstoragedevice.c @@ -0,0 +1,292 @@ +/* + * virstoragedevice.c: utility functions to share storage device mgmt + * between storage pools and domains + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + */ + +#include <config.h> + +#include "device_conf.h"
After Peter's b4c73106333c0ede this will not fly. Initially I was going to suggest: #include "conf/device_conf.h" but that will not help either. IIUC you need device_conf just for virPCIDeviceAddressParseXML(). Well, I guess we need to move that ParseXML() into src/util/virpci.c so that we can re-use it in src/util/. Or even better - why virstoragedevie needs to live in src/util if it is really just a set of XML parse/format functions? I'd expect it to live under src/conf. Kudos for rolling up your sleeves and putting all these functions in one place. Michal
On 02/27/2017 10:36 AM, Michal Privoznik wrote:
On 20.02.2017 14:18, John Ferlan wrote:
Introduce src/util/virstoragedevice.{c,h}. Rather than have virstoragefile be a repository for more things - create a new module to share the adapter definitions between the storage pool and eventually the domain. The devices will need device address functions (no need to pollute more code for that).
Move the virStoragePoolSourceAdapter from storage_conf into the new module and parcel out the the structure a bit more into 'fchost' and 'scsi_host' specific pieces creating virStoragePoolSourceAdapterSCSIHost and virStoragePoolSourceAdapterFCHost structures for easier access.
Modify code in the various places which formerly used the pool structure to use the new model. This includes some changes that will use the Ptr rather than just the struct (try to shorten the number of times one has to type adapter.data.fchost or adapter.data.scsi_host as well as [pool->]def->source.adapter.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- po/POTFILES.in | 1 + src/Makefile.am | 1 + src/conf/storage_conf.c | 338 ++++++++----------------------------- src/conf/storage_conf.h | 35 +--- src/libvirt_private.syms | 16 +- src/libxl/libxl_conf.c | 1 + src/phyp/phyp_driver.c | 3 +- src/storage/storage_backend_scsi.c | 131 +++++++------- src/test/test_driver.c | 4 +- src/util/virscsihost.c | 28 +-- src/util/virscsihost.h | 8 +- src/util/virstoragedevice.c | 292 ++++++++++++++++++++++++++++++++ src/util/virstoragedevice.h | 89 ++++++++++ 13 files changed, 548 insertions(+), 399 deletions(-) create mode 100644 src/util/virstoragedevice.c create mode 100644 src/util/virstoragedevice.h
diff --git a/src/util/virstoragedevice.c b/src/util/virstoragedevice.c new file mode 100644 index 0000000..0d9db34 --- /dev/null +++ b/src/util/virstoragedevice.c @@ -0,0 +1,292 @@ +/* + * virstoragedevice.c: utility functions to share storage device mgmt + * between storage pools and domains + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + */ + +#include <config.h> + +#include "device_conf.h"
After Peter's b4c73106333c0ede this will not fly. Initially I was going to suggest:
#include "conf/device_conf.h"
but that will not help either. IIUC you need device_conf just for virPCIDeviceAddressParseXML(). Well, I guess we need to move that ParseXML() into src/util/virpci.c so that we can re-use it in src/util/. Or even better - why virstoragedevie needs to live in src/util if it is really just a set of XML parse/format functions? I'd expect it to live under src/conf.
Yes, virPCIDeviceAddressParseXML and also virPCIDeviceAddressFormat The "thought process" is/was similarity with virstoragefile and virstorageencryption... Then yes, that pesky PCI address stuff got in the way. Much easier to include *conf than move it. If storagedevice moves to conf, then theory would say that parts of the others should follow (auth and encryption Parse/Format) - essentially things that were shared between storage and domain definitions Let's see what I can come up with for this. Of course my long term vision had a second reason - to be able to share the <adapter> parsing with the domain code which was going to get a vHBA, but that may not become a reality now based on Daniel's feelings over the event mechanism between qemu and nodedev. John
Kudos for rolling up your sleeves and putting all these functions in one place.
Michal
The function is actually in virutil.c, but prototyped in virfile.h. This patch fixes that by renaming the function to virWaitForDevices, adding the prototype in virutil.h and libvirt_private.syms, and then changing the callers to use the new name. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libvirt_private.syms | 2 +- src/node_device/node_device_driver.c | 2 +- src/storage/storage_backend_disk.c | 6 +++--- src/storage/storage_backend_iscsi.c | 2 +- src/storage/storage_backend_logical.c | 4 ++-- src/storage/storage_backend_mpath.c | 2 +- src/storage/storage_backend_scsi.c | 2 +- src/storage/storage_util.c | 2 +- src/util/virfile.h | 2 -- src/util/virutil.c | 4 ++-- src/util/virutil.h | 2 ++ 11 files changed, 15 insertions(+), 15 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index b88e700..563d6f6 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1628,7 +1628,6 @@ virFileStripSuffix; virFileTouch; virFileUnlock; virFileUpdatePerm; -virFileWaitForDevices; virFileWrapperFdClose; virFileWrapperFdFree; virFileWrapperFdNew; @@ -2743,6 +2742,7 @@ virTristateSwitchTypeFromString; virTristateSwitchTypeToString; virUpdateSelfLastChanged; virValidateWWN; +virWaitForDevices; # util/viruuid.h diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 5d57006..fcb5644 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -548,7 +548,7 @@ find_new_device(virConnectPtr conn, const char *wwnn, const char *wwpn) while ((now - start) < LINUX_NEW_DEVICE_WAIT_TIME) { - virFileWaitForDevices(); + virWaitForDevices(); dev = nodeDeviceLookupSCSIHostByWWN(conn, wwnn, wwpn, 0); diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c index 819f1e5..75d8f63 100644 --- a/src/storage/storage_backend_disk.c +++ b/src/storage/storage_backend_disk.c @@ -426,7 +426,7 @@ virStorageBackendDiskRefreshPool(virConnectPtr conn ATTRIBUTE_UNUSED, VIR_FREE(pool->def->source.devices[0].freeExtents); pool->def->source.devices[0].nfreeExtent = 0; - virFileWaitForDevices(); + virWaitForDevices(); if (!virFileExists(pool->def->source.devices[0].path)) { virReportError(VIR_ERR_INVALID_ARG, @@ -450,7 +450,7 @@ virStorageBackendDiskStartPool(virConnectPtr conn ATTRIBUTE_UNUSED, virStoragePoolFormatDiskTypeToString(pool->def->source.format); const char *path = pool->def->source.devices[0].path; - virFileWaitForDevices(); + virWaitForDevices(); if (!virFileExists(path)) { virReportError(VIR_ERR_INVALID_ARG, @@ -859,7 +859,7 @@ virStorageBackendDiskCreateVol(virConnectPtr conn, goto cleanup; /* wait for device node to show up */ - virFileWaitForDevices(); + virWaitForDevices(); /* Blow away free extent info, as we're about to re-populate it */ VIR_FREE(pool->def->source.devices[0].freeExtents); diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c index 2813341..4a5563a 100644 --- a/src/storage/storage_backend_iscsi.c +++ b/src/storage/storage_backend_iscsi.c @@ -98,7 +98,7 @@ virStorageBackendISCSIGetHostNumber(const char *sysfs_path, VIR_DEBUG("Finding host number from '%s'", sysfs_path); - virFileWaitForDevices(); + virWaitForDevices(); if (virDirOpen(&sysdir, sysfs_path) < 0) goto cleanup; diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index b0191aa..2acd0ef 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -831,7 +831,7 @@ virStorageBackendLogicalRefreshPool(virConnectPtr conn ATTRIBUTE_UNUSED, virCommandPtr cmd = NULL; int ret = -1; - virFileWaitForDevices(); + virWaitForDevices(); /* Get list of all logical volumes */ if (virStorageBackendLogicalFindLVs(pool, NULL) < 0) @@ -925,7 +925,7 @@ virStorageBackendLogicalDeleteVol(virConnectPtr conn ATTRIBUTE_UNUSED, virCheckFlags(0, -1); - virFileWaitForDevices(); + virWaitForDevices(); lvchange_cmd = virCommandNewArgList(LVCHANGE, "-aln", vol->target.path, NULL); lvremove_cmd = virCommandNewArgList(LVREMOVE, "-f", vol->target.path, NULL); diff --git a/src/storage/storage_backend_mpath.c b/src/storage/storage_backend_mpath.c index a5d692a..501e316 100644 --- a/src/storage/storage_backend_mpath.c +++ b/src/storage/storage_backend_mpath.c @@ -261,7 +261,7 @@ virStorageBackendMpathRefreshPool(virConnectPtr conn ATTRIBUTE_UNUSED, pool->def->allocation = pool->def->capacity = pool->def->available = 0; - virFileWaitForDevices(); + virWaitForDevices(); virStorageBackendGetMaps(pool); diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index afc3c89..33df1c9 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -343,7 +343,7 @@ createVport(virConnectPtr conn, VPORT_CREATE) < 0) goto cleanup; - virFileWaitForDevices(); + virWaitForDevices(); /* Creating our own VPORT didn't leave enough time to find any LUN's, * so, let's create a thread whose job it is to call the FindLU's with diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index a665bac..64fc843 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -4012,7 +4012,7 @@ virStorageBackendSCSIFindLUs(virStoragePoolObjPtr pool, VIR_DEBUG("Discovering LUs on host %u", scanhost); - virFileWaitForDevices(); + virWaitForDevices(); if (virDirOpen(&devicedir, device_path) < 0) return -1; diff --git a/src/util/virfile.h b/src/util/virfile.h index f2a3faa..b29feee 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -292,8 +292,6 @@ int virFileOpenTty(int *ttymaster, char *virFileFindMountPoint(const char *type); -void virFileWaitForDevices(void); - /* NB: this should be combined with virFileBuildPath */ # define virBuildPath(path, ...) \ virBuildPathInternal(path, __VA_ARGS__, NULL) diff --git a/src/util/virutil.c b/src/util/virutil.c index bb0f2d2..79db1d8 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -1582,7 +1582,7 @@ virSetUIDGIDWithCaps(uid_t uid, gid_t gid, gid_t *groups, int ngroups, #if defined(UDEVADM) || defined(UDEVSETTLE) -void virFileWaitForDevices(void) +void virWaitForDevices(void) { # ifdef UDEVADM const char *const settleprog[] = { UDEVADM, "settle", NULL }; @@ -1603,7 +1603,7 @@ void virFileWaitForDevices(void) ignore_value(virRun(settleprog, &exitstatus)); } #else -void virFileWaitForDevices(void) +void virWaitForDevices(void) {} #endif diff --git a/src/util/virutil.h b/src/util/virutil.h index 877207c..e097b77 100644 --- a/src/util/virutil.h +++ b/src/util/virutil.h @@ -51,6 +51,8 @@ int virSetUIDGIDWithCaps(uid_t uid, gid_t gid, gid_t *groups, int ngroups, unsigned long long capBits, bool clearExistingCaps); +void virWaitForDevices(void); + int virScaleInteger(unsigned long long *value, const char *suffix, unsigned long long scale, unsigned long long limit) ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; -- 2.9.3
On 02/20/2017 08:18 AM, John Ferlan wrote:
The function is actually in virutil.c, but prototyped in virfile.h. This patch fixes that by renaming the function to virWaitForDevices, adding the prototype in virutil.h and libvirt_private.syms, and then changing the callers to use the new name.
Signed-off-by: John Ferlan <jferlan@redhat.com>
ACK.
On 20.02.2017 14:18, John Ferlan wrote:
The function is actually in virutil.c, but prototyped in virfile.h. This patch fixes that by renaming the function to virWaitForDevices, adding the prototype in virutil.h and libvirt_private.syms, and then changing the callers to use the new name.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libvirt_private.syms | 2 +- src/node_device/node_device_driver.c | 2 +- src/storage/storage_backend_disk.c | 6 +++--- src/storage/storage_backend_iscsi.c | 2 +- src/storage/storage_backend_logical.c | 4 ++-- src/storage/storage_backend_mpath.c | 2 +- src/storage/storage_backend_scsi.c | 2 +- src/storage/storage_util.c | 2 +- src/util/virfile.h | 2 -- src/util/virutil.c | 4 ++-- src/util/virutil.h | 2 ++ 11 files changed, 15 insertions(+), 15 deletions(-)
ACK Michal
Move the bulk of the code to the node_device_conf and rename to virNodeDevice{Create|Delete}Vport. Alter the create algorithm slightly in order to return the name of the scsi_host vHBA that was created. The existing code would fetch the name and if it exists would start a thread looking for the LUNs; however, in no way did it validate the device was created for the storage pool even though it would be used thereafter. This slight change allows the code that checks if the node device by wwnn/wwpn already exists and return that name. This fixes a second yet seen issue that if the nodedev was present, but the parent by name wasn't provided (perhaps parent by wwnn/wwpn or by fabric_name), then a failure would be returned. For this path it shouldn't be an error - we should just be happy that something else is managing the device and we don't have to create/delete it. The end result is that the startVport code can now just start the thread once it gets a non NULL name back. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/node_device_conf.c | 211 +++++++++++++++++++++++++++++++++++++ src/conf/node_device_conf.h | 9 ++ src/libvirt_private.syms | 2 + src/storage/storage_backend_scsi.c | 190 +++------------------------------ 4 files changed, 235 insertions(+), 177 deletions(-) diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index 07b0838..68ed5ad 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -2344,3 +2344,214 @@ virNodeDeviceGetParentName(virConnectPtr conn, return parent; } + + +/* + * Using the host# name found via wwnn/wwpn lookup in the fc_host + * sysfs tree to get the parent 'scsi_host#' to ensure it matches. + */ +static bool +nodeDeviceCheckParent(virConnectPtr conn, + const char *name, + const char *parent_name) +{ + char *scsi_host_name = NULL; + char *vhba_parent = NULL; + bool retval = false; + + VIR_DEBUG("conn=%p, name=%s, parent_name=%s", conn, name, parent_name); + + /* autostarted pool - assume we're OK */ + if (!conn) + return true; + + if (virAsprintf(&scsi_host_name, "scsi_%s", name) < 0) + goto cleanup; + + if (!(vhba_parent = virNodeDeviceGetParentName(conn, scsi_host_name))) + goto cleanup; + + if (STRNEQ(parent_name, vhba_parent)) { + virReportError(VIR_ERR_XML_ERROR, + _("Parent attribute '%s' does not match parent '%s' " + "determined for the '%s' wwnn/wwpn lookup."), + parent_name, vhba_parent, name); + goto cleanup; + } + + retval = true; + + cleanup: + VIR_FREE(vhba_parent); + VIR_FREE(scsi_host_name); + return retval; +} + + +/** + * @conn: Connection pointer + * @fchost: Pointer to vHBA adapter + * + * Create a vHBA for Storage. This code accomplishes this via searching + * through the sysfs for scsi_host/fc_host in order to first ensure some + * vHBA doesn't already exist for the requested wwnn/wwpn (e.g. an unmanaged + * vHBA) and to search for the parent vport capable scsi_host by name, + * wwnn/wwpn, or fabric_wwn (if provided). If no parent is provided, then + * a vport capable scsi_host will be selected. + * + * Returns vHBA name on success, NULL on failure with an error message set + */ +char * +virNodeDeviceCreateVport(virConnectPtr conn, + virStorageAdapterFCHostPtr fchost) +{ + unsigned int parent_host; + char *name = NULL; + char *parent_hoststr = NULL; + bool skip_capable_check = false; + + VIR_DEBUG("conn=%p, parent='%s', wwnn='%s' wwpn='%s'", + conn, NULLSTR(fchost->parent), fchost->wwnn, fchost->wwpn); + + /* If we find an existing HBA/vHBA within the fc_host sysfs + * using the wwnn/wwpn, then a nodedev is already created for + * this pool and we don't have to create the vHBA + */ + if ((name = virVHBAGetHostByWWN(NULL, fchost->wwnn, fchost->wwpn))) { + /* If a parent was provided, let's make sure the 'name' we've + * retrieved has the same parent. If not this will cause failure. */ + if (fchost->parent && nodeDeviceCheckParent(conn, name, fchost->parent)) + VIR_FREE(name); + + return name; + } + + if (fchost->parent) { + if (VIR_STRDUP(parent_hoststr, fchost->parent) < 0) + goto cleanup; + } else if (fchost->parent_wwnn && fchost->parent_wwpn) { + if (!(parent_hoststr = virVHBAGetHostByWWN(NULL, fchost->parent_wwnn, + fchost->parent_wwpn))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("cannot find parent using provided wwnn/wwpn")); + goto cleanup; + } + } else if (fchost->parent_fabric_wwn) { + if (!(parent_hoststr = + virVHBAGetHostByFabricWWN(NULL, fchost->parent_fabric_wwn))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("cannot find parent using provided fabric_wwn")); + goto cleanup; + } + } else { + if (!(parent_hoststr = virVHBAFindVportHost(NULL))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("'parent' for vHBA not specified, and " + "cannot find one on this host")); + goto cleanup; + } + skip_capable_check = true; + } + + if (virSCSIHostGetNumber(parent_hoststr, &parent_host) < 0) + goto cleanup; + + /* NOTE: + * We do not save the parent_hoststr in fchost->parent since + * we could be writing out the 'def' to the saved XML config. + * If we wrote out the name in the XML, then future starts would + * always use the same parent rather than finding the "best available" + * parent. Besides we have a way to determine the parent based on + * the 'name' field. + */ + if (!skip_capable_check && !virVHBAPathExists(NULL, parent_host)) { + virReportError(VIR_ERR_XML_ERROR, + _("parent '%s' specified for vHBA does not exist"), + parent_hoststr); + goto cleanup; + } + + if (virVHBAManageVport(parent_host, fchost->wwpn, fchost->wwnn, + VPORT_CREATE) < 0) + goto cleanup; + + /* Let's ensure the device was created */ + virWaitForDevices(); + if (!(name = virVHBAGetHostByWWN(NULL, fchost->wwnn, fchost->wwpn))) { + ignore_value(virVHBAManageVport(parent_host, fchost->wwpn, fchost->wwnn, + VPORT_DELETE)); + goto cleanup; + } + + cleanup: + VIR_FREE(parent_hoststr); + return name; +} + + +/** + * @conn: Connection pointer + * @fchost: Pointer to vHBA adapter + * + * As long as the vHBA is being managed, search for the scsi_host via the + * provided wwnn/wwpn and then find the corresponding parent scsi_host in + * order to send the delete request. + * + * Returns 0 on success, -1 on failure + */ +int +virNodeDeviceDeleteVport(virConnectPtr conn, + virStorageAdapterFCHostPtr fchost) +{ + char *name = NULL; + char *scsi_host_name = NULL; + unsigned int parent_host; + char *vhba_parent = NULL; + int ret = -1; + + VIR_DEBUG("conn=%p parent='%s', managed='%d' wwnn='%s' wwpn='%s'", + conn, NULLSTR(fchost->parent), fchost->managed, + fchost->wwnn, fchost->wwpn); + + /* If we're not managing the deletion of the vHBA, then just return */ + if (fchost->managed != VIR_TRISTATE_BOOL_YES) + return 0; + + /* Find our vHBA by searching the fc_host sysfs tree for our wwnn/wwpn */ + if (!(name = virVHBAGetHostByWWN(NULL, fchost->wwnn, fchost->wwpn))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to find fc_host for wwnn='%s' and wwpn='%s'"), + fchost->wwnn, fchost->wwpn); + goto cleanup; + } + + /* If at startup time we provided a parent, then use that to + * get the parent_host value; otherwise, we have to determine + * the parent scsi_host which we did not save at startup time + */ + if (fchost->parent) { + if (virSCSIHostGetNumber(fchost->parent, &parent_host) < 0) + goto cleanup; + } else { + if (virAsprintf(&scsi_host_name, "scsi_%s", name) < 0) + goto cleanup; + + if (!(vhba_parent = virNodeDeviceGetParentName(conn, scsi_host_name))) + goto cleanup; + + if (virSCSIHostGetNumber(vhba_parent, &parent_host) < 0) + goto cleanup; + } + + if (virVHBAManageVport(parent_host, fchost->wwpn, fchost->wwnn, + VPORT_DELETE) < 0) + goto cleanup; + + ret = 0; + + cleanup: + VIR_FREE(name); + VIR_FREE(vhba_parent); + VIR_FREE(scsi_host_name); + return ret; +} diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h index 8213c27..9495156 100644 --- a/src/conf/node_device_conf.h +++ b/src/conf/node_device_conf.h @@ -28,8 +28,11 @@ # include "internal.h" # include "virbitmap.h" # include "virutil.h" +# include "virscsihost.h" +# include "virstoragedevice.h" # include "virthread.h" # include "virpci.h" +# include "virvhba.h" # include "device_conf.h" # include "object_event.h" @@ -340,4 +343,10 @@ int virNodeDeviceObjListExport(virConnectPtr conn, char *virNodeDeviceGetParentName(virConnectPtr conn, const char *nodedev_name); +char *virNodeDeviceCreateVport(virConnectPtr conn, + virStorageAdapterFCHostPtr fchost); + +int virNodeDeviceDeleteVport(virConnectPtr conn, + virStorageAdapterFCHostPtr fchost); + #endif /* __VIR_NODE_DEVICE_CONF_H__ */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 563d6f6..210f637 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -695,11 +695,13 @@ virNodeDevCapsDefFree; virNodeDevCapTypeFromString; virNodeDevCapTypeToString; virNodeDeviceAssignDef; +virNodeDeviceCreateVport; virNodeDeviceDefFormat; virNodeDeviceDefFree; virNodeDeviceDefParseFile; virNodeDeviceDefParseNode; virNodeDeviceDefParseString; +virNodeDeviceDeleteVport; virNodeDeviceFindByName; virNodeDeviceFindBySysfsPath; virNodeDeviceGetParentHost; diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index 33df1c9..e9747ca 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -208,57 +208,13 @@ getAdapterName(virStorageAdapterPtr adapter) return name; } -/* - * Using the host# name found via wwnn/wwpn lookup in the fc_host - * sysfs tree to get the parent 'scsi_host#' to ensure it matches. - */ -static bool -checkParent(virConnectPtr conn, - const char *name, - const char *parent_name) -{ - char *scsi_host_name = NULL; - char *vhba_parent = NULL; - bool retval = false; - - VIR_DEBUG("conn=%p, name=%s, parent_name=%s", conn, name, parent_name); - - /* autostarted pool - assume we're OK */ - if (!conn) - return true; - - if (virAsprintf(&scsi_host_name, "scsi_%s", name) < 0) - goto cleanup; - - if (!(vhba_parent = virNodeDeviceGetParentName(conn, scsi_host_name))) - goto cleanup; - - if (STRNEQ(parent_name, vhba_parent)) { - virReportError(VIR_ERR_XML_ERROR, - _("Parent attribute '%s' does not match parent '%s' " - "determined for the '%s' wwnn/wwpn lookup."), - parent_name, vhba_parent, name); - goto cleanup; - } - - retval = true; - - cleanup: - VIR_FREE(vhba_parent); - VIR_FREE(scsi_host_name); - return retval; -} - static int createVport(virConnectPtr conn, virStoragePoolDefPtr def, const char *configFile, virStorageAdapterFCHostPtr fchost) { - unsigned int parent_host; char *name = NULL; - char *parent_hoststr = NULL; - bool skip_capable_check = false; virStoragePoolFCRefreshInfoPtr cbdata = NULL; virThread thread; int ret = -1; @@ -267,66 +223,11 @@ createVport(virConnectPtr conn, conn, NULLSTR(configFile), NULLSTR(fchost->parent), fchost->wwnn, fchost->wwpn); - /* If we find an existing HBA/vHBA within the fc_host sysfs - * using the wwnn/wwpn, then a nodedev is already created for - * this pool and we don't have to create the vHBA - */ - if ((name = virVHBAGetHostByWWN(NULL, fchost->wwnn, fchost->wwpn))) { - /* If a parent was provided, let's make sure the 'name' we've - * retrieved has the same parent - */ - if (fchost->parent && checkParent(conn, name, fchost->parent)) - ret = 0; + if (!(name = virNodeDeviceCreateVport(conn, fchost))) goto cleanup; - } - if (fchost->parent) { - if (VIR_STRDUP(parent_hoststr, fchost->parent) < 0) - goto cleanup; - } else if (fchost->parent_wwnn && fchost->parent_wwpn) { - if (!(parent_hoststr = virVHBAGetHostByWWN(NULL, fchost->parent_wwnn, - fchost->parent_wwpn))) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("cannot find parent using provided wwnn/wwpn")); - goto cleanup; - } - } else if (fchost->parent_fabric_wwn) { - if (!(parent_hoststr = - virVHBAGetHostByFabricWWN(NULL, fchost->parent_fabric_wwn))) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("cannot find parent using provided fabric_wwn")); - goto cleanup; - } - } else { - if (!(parent_hoststr = virVHBAFindVportHost(NULL))) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("'parent' for vHBA not specified, and " - "cannot find one on this host")); - goto cleanup; - } - skip_capable_check = true; - } - - if (virSCSIHostGetNumber(parent_hoststr, &parent_host) < 0) - goto cleanup; - - /* NOTE: - * We do not save the parent_hoststr in fchost->parent since - * we could be writing out the 'def' to the saved XML config. - * If we wrote out the name in the XML, then future starts would - * always use the same parent rather than finding the "best available" - * parent. Besides we have a way to determine the parent based on - * the 'name' field. - */ - if (!skip_capable_check && !virVHBAPathExists(NULL, parent_host)) { - virReportError(VIR_ERR_XML_ERROR, - _("parent '%s' specified for vHBA does not exist"), - parent_hoststr); - goto cleanup; - } - - /* Since we're creating the vHBA, then we need to manage removing it + /* Since we've created the vHBA, then we need to manage removing it * as well. Since we need this setting to "live" through a libvirtd * restart, we need to save the persistent configuration. So if not * already defined as YES, then force the issue. @@ -339,28 +240,20 @@ createVport(virConnectPtr conn, } } - if (virVHBAManageVport(parent_host, fchost->wwpn, fchost->wwnn, - VPORT_CREATE) < 0) - goto cleanup; - - virWaitForDevices(); - /* Creating our own VPORT didn't leave enough time to find any LUN's, * so, let's create a thread whose job it is to call the FindLU's with * retry logic set to true. If the thread isn't created, then no big * deal since it's still possible to refresh the pool later. */ - if ((name = virVHBAGetHostByWWN(NULL, fchost->wwnn, fchost->wwpn))) { - if (VIR_ALLOC(cbdata) == 0) { - memcpy(cbdata->pool_uuid, def->uuid, VIR_UUID_BUFLEN); - VIR_STEAL_PTR(cbdata->fchost_name, name); - - if (virThreadCreate(&thread, false, virStoragePoolFCRefreshThread, - cbdata) < 0) { - /* Oh well - at least someone can still refresh afterwards */ - VIR_DEBUG("Failed to create FC Pool Refresh Thread"); - virStoragePoolFCRefreshDataFree(cbdata); - } + if (VIR_ALLOC(cbdata) == 0) { + memcpy(cbdata->pool_uuid, def->uuid, VIR_UUID_BUFLEN); + VIR_STEAL_PTR(cbdata->fchost_name, name); + + if (virThreadCreate(&thread, false, virStoragePoolFCRefreshThread, + cbdata) < 0) { + /* Oh well - at least someone can still refresh afterwards */ + VIR_DEBUG("Failed to create FC Pool Refresh Thread"); + virStoragePoolFCRefreshDataFree(cbdata); } } @@ -368,64 +261,6 @@ createVport(virConnectPtr conn, cleanup: VIR_FREE(name); - VIR_FREE(parent_hoststr); - return ret; -} - - -static int -deleteVport(virConnectPtr conn, - virStorageAdapterFCHostPtr fchost) -{ - unsigned int parent_host; - char *name = NULL; - char *scsi_host_name = NULL; - char *vhba_parent = NULL; - int ret = -1; - - VIR_DEBUG("conn=%p parent='%s', managed='%d' wwnn='%s' wwpn='%s'", - conn, NULLSTR(fchost->parent), fchost->managed, - fchost->wwnn, fchost->wwpn); - - /* If we're not managing the deletion of the vHBA, then just return */ - if (fchost->managed != VIR_TRISTATE_BOOL_YES) - return 0; - - /* Find our vHBA by searching the fc_host sysfs tree for our wwnn/wwpn */ - if (!(name = virVHBAGetHostByWWN(NULL, fchost->wwnn, fchost->wwpn))) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to find fc_host for wwnn='%s' and wwpn='%s'"), - fchost->wwnn, fchost->wwpn); - goto cleanup; - } - - /* If at startup time we provided a parent, then use that to - * get the parent_host value; otherwise, we have to determine - * the parent scsi_host which we did not save at startup time - */ - if (fchost->parent) { - if (virSCSIHostGetNumber(fchost->parent, &parent_host) < 0) - goto cleanup; - } else { - if (virAsprintf(&scsi_host_name, "scsi_%s", name) < 0) - goto cleanup; - - if (!(vhba_parent = virNodeDeviceGetParentName(conn, scsi_host_name))) - goto cleanup; - - if (virSCSIHostGetNumber(vhba_parent, &parent_host) < 0) - goto cleanup; - } - - if (virVHBAManageVport(parent_host, fchost->wwpn, fchost->wwnn, - VPORT_DELETE) < 0) - goto cleanup; - - ret = 0; - cleanup: - VIR_FREE(name); - VIR_FREE(vhba_parent); - VIR_FREE(scsi_host_name); return ret; } @@ -517,7 +352,8 @@ virStorageBackendSCSIStopPool(virConnectPtr conn, virStoragePoolObjPtr pool) { if (pool->def->source.adapter.type == VIR_STORAGE_ADAPTER_TYPE_FC_HOST) - return deleteVport(conn, &pool->def->source.adapter.data.fchost); + return virNodeDeviceDeleteVport(conn, + &pool->def->source.adapter.data.fchost); return 0; } -- 2.9.3
On 20.02.2017 14:18, John Ferlan wrote:
Move the bulk of the code to the node_device_conf and rename to virNodeDevice{Create|Delete}Vport.
Alter the create algorithm slightly in order to return the name of the scsi_host vHBA that was created. The existing code would fetch the name and if it exists would start a thread looking for the LUNs; however, in no way did it validate the device was created for the storage pool even though it would be used thereafter.
This slight change allows the code that checks if the node device by wwnn/wwpn already exists and return that name. This fixes a second yet seen issue that if the nodedev was present, but the parent by name wasn't provided (perhaps parent by wwnn/wwpn or by fabric_name), then a failure would be returned. For this path it shouldn't be an error - we should just be happy that something else is managing the device and we don't have to create/delete it.
The end result is that the startVport code can now just start the thread once it gets a non NULL name back.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/node_device_conf.c | 211 +++++++++++++++++++++++++++++++++++++ src/conf/node_device_conf.h | 9 ++ src/libvirt_private.syms | 2 + src/storage/storage_backend_scsi.c | 190 +++------------------------------ 4 files changed, 235 insertions(+), 177 deletions(-)
ACK Michal
If we have a connection pointer there's no sense walking through the sysfs in order to create/destroy the vHBA. Instead, let's make use of the node device create/destroy API's. Since we don't have to rewrite all the various parent options for the test driver in order to test whether the storage pool creation works as the node device creation has been tested already, let's just use the altered API to test the storage pool paths. Fix a "bug" in the storage pool test driver code which "assumed" testStoragePoolObjSetDefaults should fill in the configFile for both the Define/Create (persistent) and CreateXML (transient) pools by just VIR_FREE() of the pool during CreateXML. Because the configFile was filled in, during Destroy, the pool wouldn't be free causing a test using the same name pool to fail. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/node_device_conf.c | 121 ++++++++++++++++++++++++++++++++++++++++++++ src/test/test_driver.c | 6 +++ tests/fchosttest.c | 15 ++++++ 3 files changed, 142 insertions(+) diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index 68ed5ad..1d76096 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -2390,6 +2390,82 @@ nodeDeviceCheckParent(virConnectPtr conn, /** * @conn: Connection pointer + * @fchost: Pointer to the vHBA adapter + * + * If we have a valid connection, then use the node device create + * XML API rather than traversing through the sysfs to create the vHBA. + * Generate the Node Device XML using the Storage vHBA Adapter providing + * either the parent name, parent wwnn/wwpn, or parent fabric_name if + * available to the Node Device code. Since the Storage XML processing + * requires the wwnn/wwpn to be used for the vHBA to be provided (and + * not generated), we can use that as the fc_host wwnn/wwpn. This will + * allow for easier search later when we need it. + * + * Returns vHBA name on success, NULL on failure with an error message set + */ +static char * +nodeDeviceCreateNodeDeviceVport(virConnectPtr conn, + virStorageAdapterFCHostPtr fchost) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + char *vhbaStr = NULL; + virNodeDevicePtr dev = NULL; + char *name; + bool created = false; + + /* If the nodedev already knows about this vHBA, then we're not + * managing it - we'll just use it. */ + if ((dev = virNodeDeviceLookupSCSIHostByWWN(conn, fchost->wwnn, + fchost->wwpn, 0))) + goto skip_create; + + virBufferAddLit(&buf, "<device>\n"); + virBufferAdjustIndent(&buf, 2); + if (fchost->parent) + virBufferEscapeString(&buf, "<parent>%s</parent>\n", + fchost->parent); + else if (fchost->parent_wwnn && fchost->parent_wwpn) + virBufferAsprintf(&buf, "<parent wwnn='%s' wwpn='%s'/>\n", + fchost->parent_wwnn, fchost->parent_wwpn); + else if (fchost->parent_fabric_wwn) + virBufferAsprintf(&buf, "<parent fabric_wnn='%s'/>\n", + fchost->parent_fabric_wwn); + virBufferAddLit(&buf, "<capability type='scsi_host'>\n"); + virBufferAdjustIndent(&buf, 2); + virBufferAsprintf(&buf, "<capability type='fc_host' wwnn='%s' wwpn='%s'>\n", + fchost->wwnn, fchost->wwpn); + virBufferAddLit(&buf, "</capability>\n"); + virBufferAdjustIndent(&buf, -2); + virBufferAddLit(&buf, "</capability>\n"); + virBufferAdjustIndent(&buf, -2); + virBufferAddLit(&buf, "</device>\n"); + + if (!(vhbaStr = virBufferContentAndReset(&buf))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("unable to create node device XML")); + goto cleanup; + } + + if (!(dev = virNodeDeviceCreateXML(conn, vhbaStr, 0))) + goto cleanup; + created = true; + + skip_create: + if (VIR_STRDUP(name, virNodeDeviceGetName(dev)) < 0) { + /* If we created, then destroy it */ + if (created) + ignore_value(virNodeDeviceDestroy(dev)); + } + + cleanup: + VIR_FREE(vhbaStr); + virObjectUnref(dev); + return name; +} + + +/** + * @conn: Connection pointer * @fchost: Pointer to vHBA adapter * * Create a vHBA for Storage. This code accomplishes this via searching @@ -2413,6 +2489,11 @@ virNodeDeviceCreateVport(virConnectPtr conn, VIR_DEBUG("conn=%p, parent='%s', wwnn='%s' wwpn='%s'", conn, NULLSTR(fchost->parent), fchost->wwnn, fchost->wwpn); + /* If we have a connection, bypass sysfs searching and use the + * NodeDevice API's in order to perform our delete */ + if (conn) + return nodeDeviceCreateNodeDeviceVport(conn, fchost); + /* If we find an existing HBA/vHBA within the fc_host sysfs * using the wwnn/wwpn, then a nodedev is already created for * this pool and we don't have to create the vHBA @@ -2493,6 +2574,41 @@ virNodeDeviceCreateVport(virConnectPtr conn, * @conn: Connection pointer * @fchost: Pointer to vHBA adapter * + * Search for the vHBA SCSI_HOST by the wwnn/wwpn provided at creation + * and use the node device driver to destroy. + * + * Returns 0 on success, -1 on failure w/ error message. + */ +static int +nodeDeviceDeleteNodeDeviceVport(virConnectPtr conn, + virStorageAdapterFCHostPtr fchost) +{ + int ret = -1; + virNodeDevicePtr dev; + + if (!(dev = virNodeDeviceLookupSCSIHostByWWN(conn, fchost->wwnn, + fchost->wwpn, 0))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to find fc_host for wwnn='%s' and wwpn='%s'"), + fchost->wwnn, fchost->wwpn); + return -1; + } + + if (virNodeDeviceDestroy(dev) < 0) + goto cleanup; + + ret = 0; + + cleanup: + virObjectUnref(dev); + return ret; +} + + +/** + * @conn: Connection pointer + * @fchost: Pointer to vHBA adapter + * * As long as the vHBA is being managed, search for the scsi_host via the * provided wwnn/wwpn and then find the corresponding parent scsi_host in * order to send the delete request. @@ -2517,6 +2633,11 @@ virNodeDeviceDeleteVport(virConnectPtr conn, if (fchost->managed != VIR_TRISTATE_BOOL_YES) return 0; + /* If we have a connection, bypass sysfs searching and use the + * NodeDevice API's in order to perform our delete */ + if (conn) + return nodeDeviceDeleteNodeDeviceVport(conn, fchost); + /* Find our vHBA by searching the fc_host sysfs tree for our wwnn/wwpn */ if (!(name = virVHBAGetHostByWWN(NULL, fchost->wwnn, fchost->wwpn))) { virReportError(VIR_ERR_INTERNAL_ERROR, diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 5733a20..57bf09b 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -4444,6 +4444,12 @@ testStoragePoolCreateXML(virConnectPtr conn, pool = NULL; goto cleanup; } + + /* *SetDefaults fills this in for the persistent pools, but this + * would be a transient pool so remove it; otherwise, the Destroy + * code will not Remove the pool */ + VIR_FREE(pool->configFile); + pool->active = 1; event = virStoragePoolEventLifecycleNew(pool->def->name, pool->def->uuid, diff --git a/tests/fchosttest.c b/tests/fchosttest.c index dc6b9af..c024ae5 100644 --- a/tests/fchosttest.c +++ b/tests/fchosttest.c @@ -87,6 +87,18 @@ static const char test11_xml[] = " </target>" "</pool>"; +/* virStoragePoolCreateXML without any parent to find the vport capable HBA */ +static const char test12_xml[] = +"<pool type='scsi'>" +" <name>vhba_pool</name>" +" <source>" +" <adapter type='fc_host' wwnn='20000000c9831b4b' wwpn='10000000c9831b4b'/>" +" </source>" +" <target>" +" <path>/dev/disk/by-path</path>" +" </target>" +"</pool>"; + /* Test virIsVHBACapable */ static int @@ -374,6 +386,9 @@ mymain(void) if (virTestRun("manageVHBAByStoragePool-by-parent", manageVHBAByStoragePool, test11_xml) < 0) ret = -1; + if (virTestRun("manageVHBAByStoragePool-no-parent", manageVHBAByStoragePool, + test12_xml) < 0) + ret = -1; cleanup: VIR_FREE(fchost_prefix); -- 2.9.3
On 02/20/2017 08:18 AM, John Ferlan wrote:
If we have a connection pointer there's no sense walking through the sysfs in order to create/destroy the vHBA. Instead, let's make use of the node device create/destroy API's.
Since we don't have to rewrite all the various parent options for the test driver in order to test whether the storage pool creation works as the node device creation has been tested already, let's just use the altered API to test the storage pool paths.
Fix a "bug" in the storage pool test driver code which "assumed" testStoragePoolObjSetDefaults should fill in the configFile for both the Define/Create (persistent) and CreateXML (transient) pools by just VIR_FREE() of the pool during CreateXML. Because the configFile was filled in, during Destroy, the pool wouldn't be free causing a test using the same name pool to fail.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/node_device_conf.c | 121 ++++++++++++++++++++++++++++++++++++++++++++ src/test/test_driver.c | 6 +++ tests/fchosttest.c | 15 ++++++ 3 files changed, 142 insertions(+)
diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index 68ed5ad..1d76096 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -2390,6 +2390,82 @@ nodeDeviceCheckParent(virConnectPtr conn,
/** * @conn: Connection pointer + * @fchost: Pointer to the vHBA adapter + * + * If we have a valid connection, then use the node device create + * XML API rather than traversing through the sysfs to create the vHBA. + * Generate the Node Device XML using the Storage vHBA Adapter providing + * either the parent name, parent wwnn/wwpn, or parent fabric_name if + * available to the Node Device code. Since the Storage XML processing + * requires the wwnn/wwpn to be used for the vHBA to be provided (and + * not generated), we can use that as the fc_host wwnn/wwpn. This will + * allow for easier search later when we need it. + * + * Returns vHBA name on success, NULL on failure with an error message set + */ +static char * +nodeDeviceCreateNodeDeviceVport(virConnectPtr conn, + virStorageAdapterFCHostPtr fchost) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + char *vhbaStr = NULL; + virNodeDevicePtr dev = NULL; + char *name; + bool created = false; + + /* If the nodedev already knows about this vHBA, then we're not + * managing it - we'll just use it. */ + if ((dev = virNodeDeviceLookupSCSIHostByWWN(conn, fchost->wwnn, + fchost->wwpn, 0)))
I get nervous anytime I see a call to a toplevel public libvirt API inside some low level function even when it's in one of the driver directories. But this is in the conf directory. Is there a precedent for doing that? It doesn't seem like a good idea - if it's called from within any other nodedev function it could end up in a deadlock of the driver-wide lock. Is there some other organization that could make this cleaner? (haven't thought about what yet)
On 02/27/2017 01:15 PM, Laine Stump wrote:
On 02/20/2017 08:18 AM, John Ferlan wrote:
If we have a connection pointer there's no sense walking through the sysfs in order to create/destroy the vHBA. Instead, let's make use of the node device create/destroy API's.
Since we don't have to rewrite all the various parent options for the test driver in order to test whether the storage pool creation works as the node device creation has been tested already, let's just use the altered API to test the storage pool paths.
Fix a "bug" in the storage pool test driver code which "assumed" testStoragePoolObjSetDefaults should fill in the configFile for both the Define/Create (persistent) and CreateXML (transient) pools by just VIR_FREE() of the pool during CreateXML. Because the configFile was filled in, during Destroy, the pool wouldn't be free causing a test using the same name pool to fail.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/node_device_conf.c | 121 ++++++++++++++++++++++++++++++++++++++++++++ src/test/test_driver.c | 6 +++ tests/fchosttest.c | 15 ++++++ 3 files changed, 142 insertions(+)
diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index 68ed5ad..1d76096 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -2390,6 +2390,82 @@ nodeDeviceCheckParent(virConnectPtr conn, /** * @conn: Connection pointer + * @fchost: Pointer to the vHBA adapter + * + * If we have a valid connection, then use the node device create + * XML API rather than traversing through the sysfs to create the vHBA. + * Generate the Node Device XML using the Storage vHBA Adapter providing + * either the parent name, parent wwnn/wwpn, or parent fabric_name if + * available to the Node Device code. Since the Storage XML processing + * requires the wwnn/wwpn to be used for the vHBA to be provided (and + * not generated), we can use that as the fc_host wwnn/wwpn. This will + * allow for easier search later when we need it. + * + * Returns vHBA name on success, NULL on failure with an error message set + */ +static char * +nodeDeviceCreateNodeDeviceVport(virConnectPtr conn, + virStorageAdapterFCHostPtr fchost) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + char *vhbaStr = NULL; + virNodeDevicePtr dev = NULL; + char *name; + bool created = false; + + /* If the nodedev already knows about this vHBA, then we're not + * managing it - we'll just use it. */ + if ((dev = virNodeDeviceLookupSCSIHostByWWN(conn, fchost->wwnn, + fchost->wwpn, 0)))
I get nervous anytime I see a call to a toplevel public libvirt API inside some low level function even when it's in one of the driver directories. But this is in the conf directory. Is there a precedent for doing that? It doesn't seem like a good idea - if it's called from within any other nodedev function it could end up in a deadlock of the driver-wide lock.
I usually include that type of tidbit in the commit message too... One short answer is storage_conf calls virNodeDeviceGetParentName As for the deadlock concern... I have a fix for that ;-), but it's one long series to review... Look for my virPoolObj[Table]Ptr series. It needs an update, but those driver hammer locks get removed... They're replaced by a locking system similar to qemu_driver.
Is there some other organization that could make this cleaner? (haven't thought about what yet)
I'm OK with suggestions that don't need to rototill too much code. The "idea" to generate the XML on the fly was from how volumes are migrated in qemuMigrationPrecreateDisk. Since that was possible and I knew (proved by testing too) that the nodedev creation by XML was possible - that's what I figured would make all this code a bit more "organized". Additionally all this used to be part of virvhba.c (hence the util: in the original commit message), but I had to move it to node_device_conf because of the aforementioned rule of not including a conf file in util. Another goal of this series was to not rely on perusing the file system to create a vHBA - instead one would think it would be much quicker to ask nodedev since it should already have the answer. I think it would be good to not snake through directory trees, but rather rely on nodedev to provide the answer. John
Add the parent wwnn/wwpn and fabric_wwn tests for the storage pool now that we have the capability to use the node device driver to handle the searches via a connection. Signed-off-by: John Ferlan <jferlan@redhat.com> --- tests/fchosttest.c | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/tests/fchosttest.c b/tests/fchosttest.c index c024ae5..c080c9b 100644 --- a/tests/fchosttest.c +++ b/tests/fchosttest.c @@ -100,6 +100,30 @@ static const char test12_xml[] = "</pool>"; +/* virStoragePoolCreateXML parent wwnn/wwpn to find the vport capable HBA */ +static const char test13_xml[] = +"<pool type='scsi'>" +" <name>vhba_pool</name>" +" <source>" +" <adapter type='fc_host' parent_wwnn='2000000012341234' parent_wwpn='1000000012341234' wwnn='20000000c9831b4b' wwpn='10000000c9831b4b'/>" +" </source>" +" <target>" +" <path>/dev/disk/by-path</path>" +" </target>" +"</pool>"; + +/* virStoragePoolCreateXML parent fabric_wwn to find the vport capable HBA */ +static const char test14_xml[] = +"<pool type='scsi'>" +" <name>vhba_pool</name>" +" <source>" +" <adapter type='fc_host' parent_fabric_wwn='2000000043214321' wwnn='20000000c9831b4b' wwpn='10000000c9831b4b'/>" +" </source>" +" <target>" +" <path>/dev/disk/by-path</path>" +" </target>" +"</pool>"; + /* Test virIsVHBACapable */ static int test1(const void *data ATTRIBUTE_UNUSED) @@ -389,6 +413,14 @@ mymain(void) if (virTestRun("manageVHBAByStoragePool-no-parent", manageVHBAByStoragePool, test12_xml) < 0) ret = -1; + if (virTestRun("manageVHBAByStoragePool-parent-wwn", + manageVHBAByStoragePool, + test13_xml) < 0) + ret = -1; + if (virTestRun("manageVHBAByStoragePool-parent-fabric-wwn", + manageVHBAByStoragePool, + test14_xml) < 0) + ret = -1; cleanup: VIR_FREE(fchost_prefix); -- 2.9.3
Add the parsing of a vHBA controller to a domain <controller> XML. The concept of a vHBA is somewhere between a scsi_host <hostdev> and <controller>, so rather than invent a new <vhba> type, use the existing <controller> syntax and add an <adapter> element to mimic the existing storage pool 'fc_host' <adapter> element. The new element can be parsed, but is unusable. The syntax is: <adapter [parent='%s' | parent_wwnn='%s' parent_wwpn='%s' | parent_fabric_wwn='%s'] wwnn='%s' wwpn='%s'/> and is be a subelement of a <controller type='scsi' model='virtio-scsi'> element. A vHBA is only supported for the virtio-scsi. The controller will be used exclusively to pass through vHBA LUNs to the domain as they are discovered or remove them from the domain as that's discovered. Since this controller is a "virtual" device and no physical qemu -device is created for it, add checks into various places that would place a disk or hostdev LUN onto a controller to ensure the vHBA controller is not used. For now modify the qemu_command and qemu_hotplug code to error out if one of these is defined. Future patches will add support. Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/schemas/basictypes.rng | 66 ++++---- docs/schemas/domaincommon.rng | 12 +- src/conf/domain_audit.c | 32 ++++ src/conf/domain_conf.c | 180 +++++++++++++++++++-- src/conf/domain_conf.h | 2 + src/qemu/qemu_alias.c | 5 + src/qemu/qemu_command.c | 4 + src/qemu/qemu_hotplug.c | 16 ++ .../qemuxml2argv-vhba-no-parent.xml | 38 +++++ .../qemuxml2argv-vhba-parent-fabric.xml | 38 +++++ .../qemuxml2argv-vhba-parent-name.xml | 38 +++++ .../qemuxml2argv-vhba-parent-wwns.xml | 38 +++++ .../qemuxml2xmlout-vhba-no-parent.xml | 47 ++++++ .../qemuxml2xmlout-vhba-parent-fabric.xml | 47 ++++++ .../qemuxml2xmlout-vhba-parent-name.xml | 47 ++++++ .../qemuxml2xmlout-vhba-parent-wwns.xml | 47 ++++++ tests/qemuxml2xmltest.c | 9 ++ 17 files changed, 624 insertions(+), 42 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-vhba-no-parent.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-vhba-parent-fabric.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-vhba-parent-name.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-vhba-parent-wwns.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-vhba-no-parent.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-vhba-parent-fabric.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-vhba-parent-name.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-vhba-parent-wwns.xml diff --git a/docs/schemas/basictypes.rng b/docs/schemas/basictypes.rng index cc560e6..6f050cd 100644 --- a/docs/schemas/basictypes.rng +++ b/docs/schemas/basictypes.rng @@ -417,43 +417,47 @@ <attribute name='type'> <value>fc_host</value> </attribute> - <optional> - <attribute name='parent'> - <text/> - </attribute> - </optional> - <optional> - <attribute name='managed'> - <ref name="virYesNo"/> - </attribute> - </optional> - <optional> - <attribute name='parent_wwnn'> - <ref name='wwn'/> - </attribute> - </optional> - <optional> - <attribute name='parent_wwpn'> - <ref name='wwn'/> - </attribute> - </optional> - <optional> - <attribute name='parent_fabric_wwn'> - <ref name='wwn'/> - </attribute> - </optional> - <attribute name='wwnn'> - <ref name='wwn'/> - </attribute> - <attribute name='wwpn'> - <ref name='wwn'/> - </attribute> + <ref name='vhbaadapter'/> </group> </choice> <empty/> </element> </define> + <define name="vhbaadapter"> + <optional> + <attribute name='parent'> + <text/> + </attribute> + </optional> + <optional> + <attribute name='managed'> + <ref name="virYesNo"/> + </attribute> + </optional> + <optional> + <attribute name='parent_wwnn'> + <ref name='wwn'/> + </attribute> + </optional> + <optional> + <attribute name='parent_wwpn'> + <ref name='wwn'/> + </attribute> + </optional> + <optional> + <attribute name='parent_fabric_wwn'> + <ref name='wwn'/> + </attribute> + </optional> + <attribute name='wwnn'> + <ref name='wwn'/> + </attribute> + <attribute name='wwpn'> + <ref name='wwn'/> + </attribute> + </define> + <define name="isaaddress"> <optional> <attribute name="iobase"> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index c5f1013..6738ee7 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1826,7 +1826,8 @@ </choice> </attribute> </group> - <!-- scsi has an optional attribute "model" --> + <!-- scsi has an optional attribute "model" + and optional subelement "adapter" --> <group> <attribute name="type"> <value>scsi</value> @@ -1845,6 +1846,9 @@ </choice> </attribute> </optional> + <optional> + <ref name="scsiadapter"/> + </optional> </group> <!-- usb has an optional attribute "model", and optional subelements "master" and "ports" --> @@ -4841,6 +4845,12 @@ </element> </define> + <define name="scsiadapter"> + <element name='adapter'> + <ref name='vhbaadapter'/> + </element> + </define> + <define name="usbmaster"> <element name="master"> <attribute name="startport"> diff --git a/src/conf/domain_audit.c b/src/conf/domain_audit.c index 2d9ff5e..fcd2c69 100644 --- a/src/conf/domain_audit.c +++ b/src/conf/domain_audit.c @@ -516,6 +516,32 @@ virDomainAuditHostdev(virDomainObjPtr vm, virDomainHostdevDefPtr hostdev, /** + * virDomainAuditVHBA: + * @vm: domain making a change in pass-through host device + * @vhba: device being attached or removed + * @reason: one of "start", "attach", or "detach" + * @success: true if the device passthrough operation succeeded + * + * Log an audit message about an attempted device passthrough change. + */ +static void +virDomainAuditVHBA(virDomainObjPtr vm, virStorageAdapterFCHostPtr fchost, + const char *reason, bool success) +{ + char *wwstr = NULL; + + if (virAsprintfQuiet(&wwstr, "%s:%s", fchost->wwpn, fchost->wwnn) < 0) { + VIR_WARN("OOM while encoding audit message"); + return; + } + + virDomainAuditGenericDev(vm, "vhba", NULL, wwstr, reason, success); + + VIR_FREE(wwstr); +} + + +/** * virDomainAuditRedirdev: * @vm: domain making a change in pass-through host device * @redirdev: device being attached or removed @@ -864,6 +890,12 @@ virDomainAuditStart(virDomainObjPtr vm, const char *reason, bool success) virDomainAuditHostdev(vm, hostdev, "start", true); } + for (i = 0; i < vm->def->ncontrollers; i++) { + if (!vm->def->controllers[i]->fchost) + continue; + virDomainAuditVHBA(vm, vm->def->controllers[i]->fchost, "start", true); + } + for (i = 0; i < vm->def->nredirdevs; i++) { virDomainRedirdevDefPtr redirdev = vm->def->redirdevs[i]; virDomainAuditRedirdev(vm, redirdev, "start", true); diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a56ea82..c993f57 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1789,16 +1789,22 @@ virDomainControllerDefNew(virDomainControllerType type) } -void virDomainControllerDefFree(virDomainControllerDefPtr def) +void +virDomainControllerDefFree(virDomainControllerDefPtr def) { if (!def) return; + if (def->fchost) { + virStorageAdapterVHBAClear(def->fchost); + VIR_FREE(def->fchost); + } virDomainDeviceInfoClear(&def->info); VIR_FREE(def); } + virDomainFSDefPtr virDomainFSDefNew(void) { @@ -2379,6 +2385,7 @@ void virDomainHostdevDefFree(virDomainHostdevDefPtr def) VIR_FREE(def); } + void virDomainHubDefFree(virDomainHubDefPtr def) { if (!def) @@ -3338,6 +3345,7 @@ virDomainDeviceGetInfo(virDomainDeviceDefPtr device) case VIR_DOMAIN_DEVICE_LEASE: case VIR_DOMAIN_DEVICE_GRAPHICS: case VIR_DOMAIN_DEVICE_IOMMU: + case VIR_DOMAIN_DEVICE_LAST: case VIR_DOMAIN_DEVICE_NONE: break; @@ -3653,7 +3661,6 @@ virDomainDeviceInfoIterateInternal(virDomainDefPtr def, case VIR_DOMAIN_DEVICE_GRAPHICS: case VIR_DOMAIN_DEVICE_HUB: case VIR_DOMAIN_DEVICE_REDIRDEV: - case VIR_DOMAIN_DEVICE_NONE: case VIR_DOMAIN_DEVICE_SMARTCARD: case VIR_DOMAIN_DEVICE_CHR: case VIR_DOMAIN_DEVICE_MEMBALLOON: @@ -3661,10 +3668,12 @@ virDomainDeviceInfoIterateInternal(virDomainDefPtr def, case VIR_DOMAIN_DEVICE_SHMEM: case VIR_DOMAIN_DEVICE_TPM: case VIR_DOMAIN_DEVICE_PANIC: - case VIR_DOMAIN_DEVICE_LAST: case VIR_DOMAIN_DEVICE_RNG: case VIR_DOMAIN_DEVICE_MEMORY: case VIR_DOMAIN_DEVICE_IOMMU: + + case VIR_DOMAIN_DEVICE_NONE: + case VIR_DOMAIN_DEVICE_LAST: break; } #endif @@ -4169,6 +4178,10 @@ virDomainHostdevAssignAddress(virDomainXMLOptionPtr xmlopt, if (def->controllers[i]->type != VIR_DOMAIN_CONTROLLER_TYPE_SCSI) continue; + /* Do not allow assignment to the VHBA controller */ + if (def->controllers[i]->fchost) + continue; + controller++; ret = virDomainControllerSCSINextUnit(def, max_unit, def->controllers[i]->idx); @@ -4197,6 +4210,24 @@ virDomainHostdevAssignAddress(virDomainXMLOptionPtr xmlopt, } +/* Returns true if the passed in address info matches the controller address + * for a VHBA controller */ +static bool +virDomainDriveAddressIsUsedByVHBA(const virDomainDef *def, + const virDomainDeviceDriveAddress *addr) +{ + size_t i; + + for (i = 0; i < def->ncontrollers; i++) { + if (def->controllers[i]->fchost && + def->controllers[i]->idx == addr->controller) + return true; + } + + return false; +} + + static int virDomainDeviceDefPostParseInternal(virDomainDeviceDefPtr dev, const virDomainDef *def, @@ -4303,6 +4334,17 @@ virDomainDeviceDefPostParseInternal(virDomainDeviceDefPtr dev, addr->target, addr->unit); return -1; } + + /* If the assignment was to a vHBA controller fail */ + if (virDomainDriveAddressIsUsedByVHBA(def, addr)) { + virReportError(VIR_ERR_XML_ERROR, + _("SCSI host address controller='%u' " + "bus='%u' target='%u' unit='%u' cannot " + "use a vHBA controller"), + addr->controller, addr->bus, + addr->target, addr->unit); + return -1; + } } } } @@ -4318,6 +4360,16 @@ virDomainDeviceDefPostParseInternal(virDomainDeviceDefPtr dev, virDomainControllerModelSCSITypeToString(VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI)); return -1; } + + if (cdev->fchost && cdev->type != VIR_DOMAIN_CONTROLLER_TYPE_SCSI && + cdev->model != VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI) { + virReportError(VIR_ERR_XML_ERROR, + _("Invalid VHBA configuration, requires controller " + "model '%s'"), + virDomainControllerModelSCSITypeToString(VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI)); + return -1; + } + } return 0; @@ -4736,7 +4788,8 @@ virDomainDiskAddressDiskBusCompatibility(virDomainDiskBus bus, static int -virDomainDiskDefValidate(const virDomainDiskDef *disk) +virDomainDiskDefValidate(const virDomainDef *def, + const virDomainDiskDef *disk) { /* Validate LUN configuration */ if (disk->device == VIR_DOMAIN_DISK_DEVICE_LUN) { @@ -4766,6 +4819,17 @@ virDomainDiskDefValidate(const virDomainDiskDef *disk) return -1; } + /* If the disk is assigned to a controller being used for vHBA then + * reject it. + */ + if (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE && + virDomainDriveAddressIsUsedByVHBA(def, &disk->info.addr.drive)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("disk '%s' improperly configured cannot use a " + "VHBA controller"), disk->dst); + return -1; + } + return 0; } @@ -4822,7 +4886,7 @@ virDomainDeviceDefValidateInternal(const virDomainDeviceDef *dev, { switch ((virDomainDeviceType) dev->type) { case VIR_DOMAIN_DEVICE_DISK: - return virDomainDiskDefValidate(dev->data.disk); + return virDomainDiskDefValidate(def, dev->data.disk); case VIR_DOMAIN_DEVICE_REDIRDEV: return virDomainRedirdevDefValidate(def, dev->data.redirdev); @@ -4830,6 +4894,10 @@ virDomainDeviceDefValidateInternal(const virDomainDeviceDef *dev, case VIR_DOMAIN_DEVICE_NET: return virDomainNetDefValidate(dev->data.net); + case VIR_DOMAIN_DEVICE_CONTROLLER: + if (dev->data.controller->fchost) + return virStorageAdapterVHBAParseValidate(dev->data.controller->fchost); + case VIR_DOMAIN_DEVICE_LEASE: case VIR_DOMAIN_DEVICE_FS: case VIR_DOMAIN_DEVICE_INPUT: @@ -4837,7 +4905,6 @@ virDomainDeviceDefValidateInternal(const virDomainDeviceDef *dev, case VIR_DOMAIN_DEVICE_VIDEO: case VIR_DOMAIN_DEVICE_HOSTDEV: case VIR_DOMAIN_DEVICE_WATCHDOG: - case VIR_DOMAIN_DEVICE_CONTROLLER: case VIR_DOMAIN_DEVICE_GRAPHICS: case VIR_DOMAIN_DEVICE_HUB: case VIR_DOMAIN_DEVICE_SMARTCARD: @@ -6757,7 +6824,16 @@ virDomainDiskDefAssignAddress(virDomainXMLOptionPtr xmlopt, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("using disk target name '%s' conflicts with " "SCSI host device address controller='%u' " - "bus='%u' target='%u' unit='%u"), + "bus='%u' target='%u' unit='%u'"), + def->dst, controller, 0, 0, unit); + return -1; + } + + if (virDomainDriveAddressIsUsedByVHBA(vmdef, &addr)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("using disk target name '%s' conflicts with " + "VHBA controller='%u' bus='%u' target='%u' " + "unit='%u'"), def->dst, controller, 0, 0, unit); return -1; } @@ -8572,6 +8648,20 @@ virDomainControllerModelTypeToString(virDomainControllerDefPtr def, } +static int +virDomainControllerVHBAParseXML(xmlNodePtr node, + virDomainControllerDefPtr def) +{ + if (VIR_ALLOC(def->fchost) < 0) + return -1; + + if (virStorageAdapterVHBAParseXML(node, def->fchost) < 0) + return -1; + + return 0; +} + + /* Parse the XML definition for a controller * @param node XML nodeset to parse for controller definition */ @@ -8669,6 +8759,9 @@ virDomainControllerDefParseXML(xmlNodePtr node, port = virXMLPropString(cur, "port"); busNr = virXMLPropString(cur, "busNr"); processedTarget = true; + } else if (xmlStrEqual(cur->name, BAD_CAST "adapter")) { + if (virDomainControllerVHBAParseXML(cur, def) < 0) + goto error; } } cur = cur->next; @@ -18609,6 +18702,67 @@ virDomainControllerDefCheckABIStability(virDomainControllerDefPtr src, return false; } + if ((src->fchost && !dst->fchost) || (!src->fchost && dst->fchost)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Target controller VHBA presence does not match " + "source")); + return false; + } + + if (src->fchost) { + if (STRNEQ(src->fchost->wwnn, dst->fchost->wwnn)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Target VHBA wwnn %s does not match source %s"), + dst->fchost->wwnn, src->fchost->wwnn); + return false; + } + + if (STRNEQ(src->fchost->wwpn, dst->fchost->wwpn)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Target VHBA wwpn %s does not match source %s"), + dst->fchost->wwpn, src->fchost->wwpn); + return false; + } + + if (STRNEQ_NULLABLE(src->fchost->parent, dst->fchost->parent)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Target VHBA parent %s does not match source %s"), + NULLSTR(dst->fchost->parent), + NULLSTR(src->fchost->parent)); + return false; + } + + if (STRNEQ_NULLABLE(src->fchost->parent_wwnn, + dst->fchost->parent_wwnn)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Target VHBA parent wwnn %s does not match " + "source %s"), + NULLSTR(dst->fchost->parent_wwnn), + NULLSTR(src->fchost->parent_wwnn)); + return false; + } + + if (STRNEQ_NULLABLE(src->fchost->parent_wwpn, + dst->fchost->parent_wwpn)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Target VHBA parent wwpn %s does not match " + "source %s"), + NULLSTR(dst->fchost->parent_wwpn), + NULLSTR(src->fchost->parent_wwpn)); + return false; + } + + if (STRNEQ_NULLABLE(src->fchost->parent_fabric_wwn, + dst->fchost->parent_fabric_wwn)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Target VHBA parent fabric wwn %s does not match " + "source %s"), + NULLSTR(dst->fchost->parent_fabric_wwn), + NULLSTR(src->fchost->parent_fabric_wwn)); + return false; + } + } + if (src->type == VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL) { if (src->opts.vioserial.ports != dst->opts.vioserial.ports) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -19875,18 +20029,19 @@ virDomainDefCheckABIStabilityFlags(virDomainDefPtr src, case VIR_DOMAIN_DEVICE_GRAPHICS: case VIR_DOMAIN_DEVICE_HUB: case VIR_DOMAIN_DEVICE_REDIRDEV: - case VIR_DOMAIN_DEVICE_NONE: case VIR_DOMAIN_DEVICE_SMARTCARD: case VIR_DOMAIN_DEVICE_CHR: case VIR_DOMAIN_DEVICE_MEMBALLOON: case VIR_DOMAIN_DEVICE_NVRAM: - case VIR_DOMAIN_DEVICE_LAST: case VIR_DOMAIN_DEVICE_RNG: case VIR_DOMAIN_DEVICE_TPM: case VIR_DOMAIN_DEVICE_PANIC: case VIR_DOMAIN_DEVICE_SHMEM: case VIR_DOMAIN_DEVICE_MEMORY: case VIR_DOMAIN_DEVICE_IOMMU: + + case VIR_DOMAIN_DEVICE_NONE: + case VIR_DOMAIN_DEVICE_LAST: break; } #endif @@ -20900,7 +21055,7 @@ virDomainControllerDefFormat(virBufferPtr buf, if (pciModel || pciTarget || def->queues || def->cmd_per_lun || def->max_sectors || def->ioeventfd || - def->iothread || + def->iothread || def->fchost || virDomainDeviceInfoNeedsFormat(&def->info, flags) || pcihole64) { virBufferAddLit(buf, ">\n"); virBufferAdjustIndent(buf, 2); @@ -20965,6 +21120,11 @@ virDomainControllerDefFormat(virBufferPtr buf, virBufferAddLit(buf, "/>\n"); } + if (def->fchost) { + virBufferAddLit(buf, "<adapter"); + virStorageAdapterVHBAFormat(buf, def->fchost); + } + if (virDomainDeviceInfoNeedsFormat(&def->info, flags) && virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) return -1; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 7e1afa4..90ad556 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -48,6 +48,7 @@ # include "virobject.h" # include "device_conf.h" # include "virbitmap.h" +# include "virstoragedevice.h" # include "virstoragefile.h" # include "virseclabel.h" # include "virprocess.h" @@ -795,6 +796,7 @@ struct _virDomainControllerDef { virDomainPCIControllerOpts pciopts; virDomainUSBControllerOpts usbopts; } opts; + virStorageAdapterFCHostPtr fchost; virDomainDeviceInfo info; }; diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c index 8521a44..6dc9985 100644 --- a/src/qemu/qemu_alias.c +++ b/src/qemu/qemu_alias.c @@ -165,6 +165,11 @@ qemuAssignDeviceControllerAlias(virDomainDefPtr domainDef, /* first USB device is "usb", others are normal "usb%d" */ if (controller->idx == 0) return VIR_STRDUP(controller->info.alias, "usb"); + } else if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI && + controller->fchost) { + /* There is no need for an alias for a vHBA since we're not passing + * it through to QEMU */ + return 0; } /* all other controllers use the default ${type}${index} naming * scheme for alias/id. diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index f4bcfd4..44c2c81 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3120,6 +3120,10 @@ qemuBuildControllerDevCommandLine(virCommandPtr cmd, if (cont->type != contOrder[j]) continue; + /* skip vHBA controller */ + if (cont->fchost) + continue; + /* skip USB controllers with type none.*/ if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_USB && cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE) { diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 2f209f1..f7829da 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -481,6 +481,12 @@ int qemuDomainAttachControllerDevice(virQEMUDriverPtr driver, return -1; } + if (controller->fchost) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("hotplug not support for vHBA controller.")); + return -1; + } + /* default idx would normally be set by virDomainDefPostParse(), * which isn't called in the case of live attach of a single * device. @@ -566,6 +572,10 @@ qemuDomainFindOrCreateSCSIDiskController(virQEMUDriverPtr driver, if (cont->type != VIR_DOMAIN_CONTROLLER_TYPE_SCSI) continue; + /* The VHBA controller has a singular purpose */ + if (cont->fchost) + continue; + if (cont->idx == controller) return cont; } @@ -4597,6 +4607,12 @@ int qemuDomainDetachControllerDevice(virQEMUDriverPtr driver, goto cleanup; } + if (detach->fchost) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("vHBA controller cannot be detached")); + goto cleanup; + } + if (!virDomainDeviceAddressIsValid(&detach->info, detach->info.type)) { virReportError(VIR_ERR_OPERATION_FAILED, _("device with invalid '%s' address cannot be detached"), diff --git a/tests/qemuxml2argvdata/qemuxml2argv-vhba-no-parent.xml b/tests/qemuxml2argvdata/qemuxml2argv-vhba-no-parent.xml new file mode 100644 index 0000000..20ef29c --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-vhba-no-parent.xml @@ -0,0 +1,38 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <disk type='file' device='disk'> + <source file='/tmp/scsidisk.img'/> + <target dev='sda' bus='scsi'/> + <address type='drive' controller='0' bus='0' target='4' unit='0'/> + </disk> + <controller type='ide' index='0'/> + <controller type='scsi' index='0' model='virtio-scsi'/> + <controller type='scsi' index='1' model='virtio-scsi'> + <adapter wwnn='20000000c9831b4b' wwpn='10000000c9831b4b'/> + </controller> + <controller type='usb' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-vhba-parent-fabric.xml b/tests/qemuxml2argvdata/qemuxml2argv-vhba-parent-fabric.xml new file mode 100644 index 0000000..b58f924 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-vhba-parent-fabric.xml @@ -0,0 +1,38 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <disk type='file' device='disk'> + <source file='/tmp/scsidisk.img'/> + <target dev='sda' bus='scsi'/> + <address type='drive' controller='0' bus='0' target='4' unit='0'/> + </disk> + <controller type='ide' index='0'/> + <controller type='scsi' index='0' model='virtio-scsi'/> + <controller type='scsi' index='1' model='virtio-scsi'> + <adapter parent_fabric_wwn='2000000043214321' wwnn='20000000c9831b4b' wwpn='10000000c9831b4b'/> + </controller> + <controller type='usb' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-vhba-parent-name.xml b/tests/qemuxml2argvdata/qemuxml2argv-vhba-parent-name.xml new file mode 100644 index 0000000..539188c --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-vhba-parent-name.xml @@ -0,0 +1,38 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <disk type='file' device='disk'> + <source file='/tmp/scsidisk.img'/> + <target dev='sda' bus='scsi'/> + <address type='drive' controller='0' bus='0' target='4' unit='0'/> + </disk> + <controller type='ide' index='0'/> + <controller type='scsi' index='0' model='virtio-scsi'/> + <controller type='scsi' index='1' model='virtio-scsi'> + <adapter parent='scsi_host1' wwnn='20000000c9831b4b' wwpn='10000000c9831b4b'/> + </controller> + <controller type='usb' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-vhba-parent-wwns.xml b/tests/qemuxml2argvdata/qemuxml2argv-vhba-parent-wwns.xml new file mode 100644 index 0000000..5ca1b80 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-vhba-parent-wwns.xml @@ -0,0 +1,38 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <disk type='file' device='disk'> + <source file='/tmp/scsidisk.img'/> + <target dev='sda' bus='scsi'/> + <address type='drive' controller='0' bus='0' target='4' unit='0'/> + </disk> + <controller type='ide' index='0'/> + <controller type='scsi' index='0' model='virtio-scsi'/> + <controller type='scsi' index='1' model='virtio-scsi'> + <adapter parent_wwnn='2000000012341234' parent_wwpn='1000000012341234' wwnn='20000000c9831b4b' wwpn='10000000c9831b4b'/> + </controller> + <controller type='usb' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-vhba-no-parent.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-vhba-no-parent.xml new file mode 100644 index 0000000..9f5ad6b --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-vhba-no-parent.xml @@ -0,0 +1,47 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <disk type='file' device='disk'> + <source file='/tmp/scsidisk.img'/> + <target dev='sda' bus='scsi'/> + <address type='drive' controller='0' bus='0' target='4' unit='0'/> + </disk> + <controller type='ide' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> + <controller type='scsi' index='0' model='virtio-scsi'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </controller> + <controller type='scsi' index='1' model='virtio-scsi'> + <adapter wwnn='20000000c9831b4b' wwpn='10000000c9831b4b'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> + </controller> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'/> + </memballoon> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-vhba-parent-fabric.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-vhba-parent-fabric.xml new file mode 100644 index 0000000..753fe78 --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-vhba-parent-fabric.xml @@ -0,0 +1,47 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <disk type='file' device='disk'> + <source file='/tmp/scsidisk.img'/> + <target dev='sda' bus='scsi'/> + <address type='drive' controller='0' bus='0' target='4' unit='0'/> + </disk> + <controller type='ide' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> + <controller type='scsi' index='0' model='virtio-scsi'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </controller> + <controller type='scsi' index='1' model='virtio-scsi'> + <adapter parent_fabric_wwn='2000000043214321' wwnn='20000000c9831b4b' wwpn='10000000c9831b4b'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> + </controller> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'/> + </memballoon> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-vhba-parent-name.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-vhba-parent-name.xml new file mode 100644 index 0000000..e7bce0a --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-vhba-parent-name.xml @@ -0,0 +1,47 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <disk type='file' device='disk'> + <source file='/tmp/scsidisk.img'/> + <target dev='sda' bus='scsi'/> + <address type='drive' controller='0' bus='0' target='4' unit='0'/> + </disk> + <controller type='ide' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> + <controller type='scsi' index='0' model='virtio-scsi'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </controller> + <controller type='scsi' index='1' model='virtio-scsi'> + <adapter parent='scsi_host1' wwnn='20000000c9831b4b' wwpn='10000000c9831b4b'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> + </controller> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'/> + </memballoon> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-vhba-parent-wwns.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-vhba-parent-wwns.xml new file mode 100644 index 0000000..2842f08 --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-vhba-parent-wwns.xml @@ -0,0 +1,47 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <disk type='file' device='disk'> + <source file='/tmp/scsidisk.img'/> + <target dev='sda' bus='scsi'/> + <address type='drive' controller='0' bus='0' target='4' unit='0'/> + </disk> + <controller type='ide' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> + <controller type='scsi' index='0' model='virtio-scsi'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </controller> + <controller type='scsi' index='1' model='virtio-scsi'> + <adapter parent_wwnn='2000000012341234' parent_wwpn='1000000012341234' wwnn='20000000c9831b4b' wwpn='10000000c9831b4b'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> + </controller> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'/> + </memballoon> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 0702f58..e810afd 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -638,6 +638,15 @@ mymain(void) DO_TEST("disk-source-pool", NONE); DO_TEST("disk-source-pool-mode", NONE); + DO_TEST("vhba-no-parent", + QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_VIRTIO_SCSI); + DO_TEST("vhba-parent-name", + QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_VIRTIO_SCSI); + DO_TEST("vhba-parent-wwns", + QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_VIRTIO_SCSI); + DO_TEST("vhba-parent-fabric", + QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_VIRTIO_SCSI); + DO_TEST("disk-drive-discard", NONE); DO_TEST("disk-drive-detect-zeroes", NONE); -- 2.9.3
ping? Don't bother with the 8th patch since that seems to be a moot point now given that a callback mechanism between qemu and nodedev seems to have been taken off the table after Daniel and Paolo's discussion. Tks - John On 02/20/2017 08:18 AM, John Ferlan wrote:
Similar to the recent node device focused vHBA patches, but these focus more on adjustments to the Storage Pool vHBA logic and finally the mechanism to define a vHBA for a domain.
The first patch fixes a leak found by coverity that showed up because node_device_conf had enough changes so that coverity looked harder...
The second patch creates a mechanism to mock creation of the vHBA in order to test the ability for the storage pool to create a vHBA. As much as I dislike forward refs for testNodeDeviceMockCreateVport, it was better than moving all the code....
The third patch extracts out storage device mgmt into it's own set of src/util API's - similar to the existing virstoragefile, but for devices.
The fourth patch was uncovered while moving code from storage_backend_scsi into node_device_conf (the fifth patch)...
The fifth patch moves the createVport/deleteVport guts into the node_device_conf (although they could have moved to virvhba)...
The sixth patch alters the logic to use the node_device API's as the "preferred" mechanism to create/delete the vport...
The seventh patch tests the storage pool vHBA creation algorithms.
The eigth patch is the reason for all this stirring of the pot. Alter the domain <controller> XML in order to allow definition of a vHBA which more or less sits between a "scsi_hostX" host device and a controller. This is in preparation for https://bugzilla.redhat.com/show_bug.cgi?id=1404962 which can take that created vHBA and automagically add the LUNs from the vHBA to the domain although that requires a bit more magic for which there are already onlist patches to let qemu driver know when a node device has been added/removed. Once all that's in place - the next step will be to converge the two sets of patches. It's a chicken/egg type problem - one has to exist before the other can truly work.
John Ferlan (8): conf: Fix leak in virNodeDeviceDefParseXML tests: Add createVHBAByStoragePool-by-parent to fchosttest util: Convert virStoragePoolSourceAdapter to virStorageAdapter util: Rename virFileWaitForDevices storage: Move/rename createVport and deleteVport util: Alter virNodeDevice{Create|Delete}Vport to use nodedev APIs tests: Add more storage pool vHBA tests conf: Add vHBA controller definition to domain
docs/schemas/basictypes.rng | 66 ++-- docs/schemas/domaincommon.rng | 12 +- po/POTFILES.in | 1 + src/Makefile.am | 1 + src/conf/domain_audit.c | 32 ++ src/conf/domain_conf.c | 180 ++++++++++- src/conf/domain_conf.h | 2 + src/conf/node_device_conf.c | 342 ++++++++++++++++++++- src/conf/node_device_conf.h | 9 + src/conf/storage_conf.c | 338 +++++--------------- src/conf/storage_conf.h | 35 +-- src/libvirt_private.syms | 20 +- src/libxl/libxl_conf.c | 1 + src/node_device/node_device_driver.c | 2 +- src/phyp/phyp_driver.c | 3 +- src/qemu/qemu_alias.c | 5 + src/qemu/qemu_command.c | 4 + src/qemu/qemu_hotplug.c | 16 + src/storage/storage_backend_disk.c | 6 +- src/storage/storage_backend_iscsi.c | 2 +- src/storage/storage_backend_logical.c | 4 +- src/storage/storage_backend_mpath.c | 2 +- src/storage/storage_backend_scsi.c | 267 +++------------- src/storage/storage_util.c | 2 +- src/test/test_driver.c | 101 +++++- src/util/virfile.h | 2 - src/util/virscsihost.c | 28 +- src/util/virscsihost.h | 8 +- src/util/virstoragedevice.c | 292 ++++++++++++++++++ src/util/virstoragedevice.h | 89 ++++++ src/util/virutil.c | 4 +- src/util/virutil.h | 2 + tests/fchosttest.c | 111 +++++++ .../qemuxml2argv-vhba-no-parent.xml | 38 +++ .../qemuxml2argv-vhba-parent-fabric.xml | 38 +++ .../qemuxml2argv-vhba-parent-name.xml | 38 +++ .../qemuxml2argv-vhba-parent-wwns.xml | 38 +++ .../qemuxml2xmlout-vhba-no-parent.xml | 47 +++ .../qemuxml2xmlout-vhba-parent-fabric.xml | 47 +++ .../qemuxml2xmlout-vhba-parent-name.xml | 47 +++ .../qemuxml2xmlout-vhba-parent-wwns.xml | 47 +++ tests/qemuxml2xmltest.c | 9 + 42 files changed, 1727 insertions(+), 611 deletions(-) create mode 100644 src/util/virstoragedevice.c create mode 100644 src/util/virstoragedevice.h create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-vhba-no-parent.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-vhba-parent-fabric.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-vhba-parent-name.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-vhba-parent-wwns.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-vhba-no-parent.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-vhba-parent-fabric.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-vhba-parent-name.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-vhba-parent-wwns.xml
participants (3)
-
John Ferlan -
Laine Stump -
Michal Privoznik