[libvirt] [PATCH 00/24] hostdev: Cleanups and fixes
Part of an ongoing quest to fix: libvirt management of VFIO devices can lead to host crashes https://bugzilla.redhat.com/show_bug.cgi?id=1272300 Previous episode: [PATCH v2 0/9] PCI hostdev refactoring https://www.redhat.com/archives/libvir-list/2016-January/msg01066.html The plan: Once this is in, rebase the series mentioned above on top of it; hopefully it will be smaller and easier to review. Once *that* is in, rebase / rewrite the actual bug fix - probably the latter, because the code has changed a lot since I posted it In this episode: 01-06 Cleanups in the test cases; mostly unrelated, but commit 24 depends on them 07-17 More cleanups and (I believe) farily uncontroversial changes, aimed at making the actual changes either smaller or more obvious 18-19 Change in variable names that make the intended semantics clearer, which should in turn make the follow-up changes easier to review Up until here there should be basically no change in behavior 20 Bug fix 21 Improvement to error reporting 22 The whole point of this series, and also the commit where it's more likely that I might have gotten something wrong :) I haven't been able to figure out a way to split it into smaller chunks, sorry 23 Another bug fix 24 Test the changes made by the rest of the series Cheers. Andrea Bolognani (24): tests: hostdev: Remove magic numbers tests: hostdev: Use better variable names tests: hostdev: Declare count inside CHECK_LIST_COUNT() tests: hostdev: Use size_t for count variables tests: hostdev: Add more checks on list size tests: hostdev: Group test cases hostdev: Make comments easier to change later hostdev: Remove false comment hostdev: Fix indentation hostdev: Remove explicit NULL checks hostdev: Remove redundant check hostdev: virHostdevIsPCINetDevice() should return a bool hostdev: Change argument order for virHostdevReattachPCIDevice() hostdev: Look up devices using IDs when possible hostdev: Remove virHostdevGetActivePCIHostDeviceList() hostdev: Rename hostdev_mgr -> mgr hostdev: Rename usesVfio -> usesVFIO hostdev: Use consistent variable names hostdev: Add more comments hostdev: Save netdev configuration of actual device hostdev: Stop early if unmanaged devices have not been detached hostdev: Streamline device ownership tracking hostdev: Use actual device when reattaching tests: hostdev: Add more tests src/util/virhostdev.c | 660 +++++++++++++++++++++++++------------------------ tests/virhostdevtest.c | 320 ++++++++++++++++++------ 2 files changed, 576 insertions(+), 404 deletions(-) -- 2.5.0
When checking the number of devices added to a device list, use the nhostdevs variable instead of its value, so that the test can keep working even if more hostdevs are added. --- tests/virhostdevtest.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/tests/virhostdevtest.c b/tests/virhostdevtest.c index cadb66a..fa6a4fb 100644 --- a/tests/virhostdevtest.c +++ b/tests/virhostdevtest.c @@ -181,8 +181,8 @@ testVirHostdevPreparePCIHostdevs_unmanaged(const void *opaque ATTRIBUTE_UNUSED) if (virHostdevPreparePCIDevices(mgr, drv_name, dom_name, uuid, hostdevs, nhostdevs, 0) < 0) goto cleanup; - CHECK_LIST_COUNT(mgr->activePCIHostdevs, count1 + 3); - CHECK_LIST_COUNT(mgr->inactivePCIHostdevs, count2 - 3); + CHECK_LIST_COUNT(mgr->activePCIHostdevs, count1 + nhostdevs); + CHECK_LIST_COUNT(mgr->inactivePCIHostdevs, count2 - nhostdevs); /* Test conflict */ count1 = virPCIDeviceListCount(mgr->activePCIHostdevs); @@ -239,8 +239,8 @@ testVirHostdevReAttachPCIHostdevs_unmanaged(const void *opaque ATTRIBUTE_UNUSED) VIR_DEBUG("Test >=1 unmanaged hostdevs"); virHostdevReAttachPCIDevices(mgr, drv_name, dom_name, hostdevs, nhostdevs, NULL); - CHECK_LIST_COUNT(mgr->activePCIHostdevs, count1 - 3); - CHECK_LIST_COUNT(mgr->inactivePCIHostdevs, count2 + 3); + CHECK_LIST_COUNT(mgr->activePCIHostdevs, count1 - nhostdevs); + CHECK_LIST_COUNT(mgr->inactivePCIHostdevs, count2 + nhostdevs); ret = 0; @@ -266,7 +266,7 @@ testVirHostdevPreparePCIHostdevs_managed(const void *opaque ATTRIBUTE_UNUSED) if (virHostdevPreparePCIDevices(mgr, drv_name, dom_name, uuid, hostdevs, nhostdevs, 0) < 0) goto cleanup; - CHECK_LIST_COUNT(mgr->activePCIHostdevs, count1 + 3); + CHECK_LIST_COUNT(mgr->activePCIHostdevs, count1 + nhostdevs); /* Test conflict */ count1 = virPCIDeviceListCount(mgr->activePCIHostdevs); @@ -318,7 +318,7 @@ testVirHostdevReAttachPCIHostdevs_managed(const void *opaque ATTRIBUTE_UNUSED) VIR_DEBUG("Test >=1 hostdevs"); virHostdevReAttachPCIDevices(mgr, drv_name, dom_name, hostdevs, nhostdevs, NULL); - CHECK_LIST_COUNT(mgr->activePCIHostdevs, count1 - 3); + CHECK_LIST_COUNT(mgr->activePCIHostdevs, count1 - nhostdevs); ret = 0; @@ -403,7 +403,7 @@ testVirHostdevUpdateActivePCIHostdevs(const void *opaque ATTRIBUTE_UNUSED) if (virHostdevUpdateActivePCIDevices(mgr, hostdevs, nhostdevs, drv_name, dom_name) < 0) goto cleanup; - CHECK_LIST_COUNT(mgr->activePCIHostdevs, count1 + 3); + CHECK_LIST_COUNT(mgr->activePCIHostdevs, count1 + nhostdevs); ret = 0; -- 2.5.0
On 03/07/2016 12:24 PM, Andrea Bolognani wrote:
When checking the number of devices added to a device list, use the nhostdevs variable instead of its value, so that the test can keep working even if more hostdevs are added. --- tests/virhostdevtest.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
ACK.
Change the extremely generic count1 and count2 to the more descriptive active_count and inactive_count. --- tests/virhostdevtest.c | 82 +++++++++++++++++++++++++------------------------- 1 file changed, 41 insertions(+), 41 deletions(-) diff --git a/tests/virhostdevtest.c b/tests/virhostdevtest.c index fa6a4fb..febb2c9 100644 --- a/tests/virhostdevtest.c +++ b/tests/virhostdevtest.c @@ -161,52 +161,52 @@ testVirHostdevPreparePCIHostdevs_unmanaged(const void *opaque ATTRIBUTE_UNUSED) { int ret = -1; size_t i; - int count, count1, count2; + int count, active_count, inactive_count; for (i = 0; i < nhostdevs; i++) hostdevs[i]->managed = false; - count1 = virPCIDeviceListCount(mgr->activePCIHostdevs); - count2 = virPCIDeviceListCount(mgr->inactivePCIHostdevs); + active_count = virPCIDeviceListCount(mgr->activePCIHostdevs); + inactive_count = virPCIDeviceListCount(mgr->inactivePCIHostdevs); /* Test normal functionality */ VIR_DEBUG("Test 0 hostdevs"); if (virHostdevPreparePCIDevices(mgr, drv_name, dom_name, uuid, NULL, 0, 0) < 0) goto cleanup; - CHECK_LIST_COUNT(mgr->activePCIHostdevs, count1); + CHECK_LIST_COUNT(mgr->activePCIHostdevs, active_count); /* Test unmanaged hostdevs */ VIR_DEBUG("Test >=1 unmanaged hostdevs"); if (virHostdevPreparePCIDevices(mgr, drv_name, dom_name, uuid, hostdevs, nhostdevs, 0) < 0) goto cleanup; - CHECK_LIST_COUNT(mgr->activePCIHostdevs, count1 + nhostdevs); - CHECK_LIST_COUNT(mgr->inactivePCIHostdevs, count2 - nhostdevs); + CHECK_LIST_COUNT(mgr->activePCIHostdevs, active_count + nhostdevs); + CHECK_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count - nhostdevs); /* Test conflict */ - count1 = virPCIDeviceListCount(mgr->activePCIHostdevs); - count2 = virPCIDeviceListCount(mgr->inactivePCIHostdevs); + active_count = virPCIDeviceListCount(mgr->activePCIHostdevs); + inactive_count = virPCIDeviceListCount(mgr->inactivePCIHostdevs); VIR_DEBUG("Test: prepare same hostdevs for same driver/domain again"); if (!virHostdevPreparePCIDevices(mgr, drv_name, dom_name, uuid, &hostdevs[0], 1, 0)) goto cleanup; - CHECK_LIST_COUNT(mgr->activePCIHostdevs, count1); - CHECK_LIST_COUNT(mgr->inactivePCIHostdevs, count2); + CHECK_LIST_COUNT(mgr->activePCIHostdevs, active_count); + CHECK_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); VIR_DEBUG("Test: prepare same hostdevs for same driver, diff domain again"); if (!virHostdevPreparePCIDevices(mgr, drv_name, "test_domain1", uuid, &hostdevs[1], 1, 0)) goto cleanup; - CHECK_LIST_COUNT(mgr->activePCIHostdevs, count1); - CHECK_LIST_COUNT(mgr->inactivePCIHostdevs, count2); + CHECK_LIST_COUNT(mgr->activePCIHostdevs, active_count); + CHECK_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); VIR_DEBUG("Test: prepare same hostdevs for diff driver/domain again"); if (!virHostdevPreparePCIDevices(mgr, "test_driver1", dom_name, uuid, &hostdevs[2], 1, 0)) goto cleanup; - CHECK_LIST_COUNT(mgr->activePCIHostdevs, count1); - CHECK_LIST_COUNT(mgr->inactivePCIHostdevs, count2); + CHECK_LIST_COUNT(mgr->activePCIHostdevs, active_count); + CHECK_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); ret = 0; @@ -220,7 +220,7 @@ testVirHostdevReAttachPCIHostdevs_unmanaged(const void *opaque ATTRIBUTE_UNUSED) { int ret = -1; size_t i; - int count, count1, count2; + int count, active_count, inactive_count; for (i = 0; i < nhostdevs; i++) { if (hostdevs[i]->managed != false) { @@ -229,18 +229,18 @@ testVirHostdevReAttachPCIHostdevs_unmanaged(const void *opaque ATTRIBUTE_UNUSED) } } - count1 = virPCIDeviceListCount(mgr->activePCIHostdevs); - count2 = virPCIDeviceListCount(mgr->inactivePCIHostdevs); + active_count = virPCIDeviceListCount(mgr->activePCIHostdevs); + inactive_count = virPCIDeviceListCount(mgr->inactivePCIHostdevs); VIR_DEBUG("Test 0 hostdevs"); virHostdevReAttachPCIDevices(mgr, drv_name, dom_name, NULL, 0, NULL); - CHECK_LIST_COUNT(mgr->activePCIHostdevs, count1); + CHECK_LIST_COUNT(mgr->activePCIHostdevs, active_count); VIR_DEBUG("Test >=1 unmanaged hostdevs"); virHostdevReAttachPCIDevices(mgr, drv_name, dom_name, hostdevs, nhostdevs, NULL); - CHECK_LIST_COUNT(mgr->activePCIHostdevs, count1 - nhostdevs); - CHECK_LIST_COUNT(mgr->inactivePCIHostdevs, count2 + nhostdevs); + CHECK_LIST_COUNT(mgr->activePCIHostdevs, active_count - nhostdevs); + CHECK_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count + nhostdevs); ret = 0; @@ -254,39 +254,39 @@ testVirHostdevPreparePCIHostdevs_managed(const void *opaque ATTRIBUTE_UNUSED) { int ret = -1; size_t i; - int count, count1; + int count, active_count; for (i = 0; i < nhostdevs; i++) hostdevs[i]->managed = true; - count1 = virPCIDeviceListCount(mgr->activePCIHostdevs); + active_count = virPCIDeviceListCount(mgr->activePCIHostdevs); /* Test normal functionality */ VIR_DEBUG("Test >=1 hostdevs"); if (virHostdevPreparePCIDevices(mgr, drv_name, dom_name, uuid, hostdevs, nhostdevs, 0) < 0) goto cleanup; - CHECK_LIST_COUNT(mgr->activePCIHostdevs, count1 + nhostdevs); + CHECK_LIST_COUNT(mgr->activePCIHostdevs, active_count + nhostdevs); /* Test conflict */ - count1 = virPCIDeviceListCount(mgr->activePCIHostdevs); + active_count = virPCIDeviceListCount(mgr->activePCIHostdevs); VIR_DEBUG("Test: prepare same hostdevs for same driver/domain again"); if (!virHostdevPreparePCIDevices(mgr, drv_name, dom_name, uuid, &hostdevs[0], 1, 0)) goto cleanup; - CHECK_LIST_COUNT(mgr->activePCIHostdevs, count1); + CHECK_LIST_COUNT(mgr->activePCIHostdevs, active_count); VIR_DEBUG("Test: prepare same hostdevs for same driver, diff domain again"); if (!virHostdevPreparePCIDevices(mgr, drv_name, "test_domain1", uuid, &hostdevs[1], 1, 0)) goto cleanup; - CHECK_LIST_COUNT(mgr->activePCIHostdevs, count1); + CHECK_LIST_COUNT(mgr->activePCIHostdevs, active_count); VIR_DEBUG("Test: prepare same hostdevs for diff driver/domain again"); if (!virHostdevPreparePCIDevices(mgr, "test_driver1", dom_name, uuid, &hostdevs[2], 1, 0)) goto cleanup; - CHECK_LIST_COUNT(mgr->activePCIHostdevs, count1); + CHECK_LIST_COUNT(mgr->activePCIHostdevs, active_count); ret = 0; @@ -300,7 +300,7 @@ testVirHostdevReAttachPCIHostdevs_managed(const void *opaque ATTRIBUTE_UNUSED) { int ret = -1; size_t i; - int count, count1; + int count, active_count; for (i = 0; i < nhostdevs; i++) { if (hostdevs[i]->managed != true) { @@ -309,16 +309,16 @@ testVirHostdevReAttachPCIHostdevs_managed(const void *opaque ATTRIBUTE_UNUSED) } } - count1 = virPCIDeviceListCount(mgr->activePCIHostdevs); + active_count = virPCIDeviceListCount(mgr->activePCIHostdevs); VIR_DEBUG("Test 0 hostdevs"); virHostdevReAttachPCIDevices(mgr, drv_name, dom_name, NULL, 0, NULL); - CHECK_LIST_COUNT(mgr->activePCIHostdevs, count1); + CHECK_LIST_COUNT(mgr->activePCIHostdevs, active_count); VIR_DEBUG("Test >=1 hostdevs"); virHostdevReAttachPCIDevices(mgr, drv_name, dom_name, hostdevs, nhostdevs, NULL); - CHECK_LIST_COUNT(mgr->activePCIHostdevs, count1 - nhostdevs); + CHECK_LIST_COUNT(mgr->activePCIHostdevs, active_count - nhostdevs); ret = 0; @@ -332,13 +332,13 @@ testVirHostdevDetachPCINodeDevice(const void *opaque ATTRIBUTE_UNUSED) { int ret = -1; size_t i; - int count, count1; + int count, inactive_count; for (i = 0; i < nhostdevs; i++) { - count1 = virPCIDeviceListCount(mgr->inactivePCIHostdevs); + inactive_count = virPCIDeviceListCount(mgr->inactivePCIHostdevs); if (virHostdevPCINodeDeviceDetach(mgr, dev[i]) < 0) goto cleanup; - CHECK_LIST_COUNT(mgr->inactivePCIHostdevs, count1 + 1); + CHECK_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count + 1); } ret = 0; @@ -369,13 +369,13 @@ testVirHostdevReAttachPCINodeDevice(const void *opaque ATTRIBUTE_UNUSED) { int ret = -1; size_t i; - int count, count1; + int count, inactive_count; for (i = 0; i < nhostdevs; i++) { - count1 = virPCIDeviceListCount(mgr->inactivePCIHostdevs); + inactive_count = virPCIDeviceListCount(mgr->inactivePCIHostdevs); if (virHostdevPCINodeDeviceReAttach(mgr, dev[i]) < 0) goto cleanup; - CHECK_LIST_COUNT(mgr->inactivePCIHostdevs, count1 - 1); + CHECK_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count - 1); } ret = 0; @@ -389,21 +389,21 @@ static int testVirHostdevUpdateActivePCIHostdevs(const void *opaque ATTRIBUTE_UNUSED) { int ret = -1; - int count, count1; + int count, active_count; - count1 = virPCIDeviceListCount(mgr->activePCIHostdevs); + active_count = virPCIDeviceListCount(mgr->activePCIHostdevs); VIR_DEBUG("Test 0 hostdevs"); if (virHostdevUpdateActivePCIDevices(mgr, NULL, 0, drv_name, dom_name) < 0) goto cleanup; - CHECK_LIST_COUNT(mgr->activePCIHostdevs, count1); + CHECK_LIST_COUNT(mgr->activePCIHostdevs, active_count); VIR_DEBUG("Test >=1 hostdevs"); if (virHostdevUpdateActivePCIDevices(mgr, hostdevs, nhostdevs, drv_name, dom_name) < 0) goto cleanup; - CHECK_LIST_COUNT(mgr->activePCIHostdevs, count1 + nhostdevs); + CHECK_LIST_COUNT(mgr->activePCIHostdevs, active_count + nhostdevs); ret = 0; -- 2.5.0
On 03/07/2016 12:24 PM, Andrea Bolognani wrote:
Change the extremely generic count1 and count2 to the more descriptive active_count and inactive_count. --- tests/virhostdevtest.c | 82 +++++++++++++++++++++++++------------------------- 1 file changed, 41 insertions(+), 41 deletions(-)
ACK.
This variable is only used internally by the macro, so it's better to move its definition inside the macro as well. --- tests/virhostdevtest.c | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/tests/virhostdevtest.c b/tests/virhostdevtest.c index febb2c9..77b119b 100644 --- a/tests/virhostdevtest.c +++ b/tests/virhostdevtest.c @@ -37,13 +37,16 @@ VIR_LOG_INIT("tests.hostdevtest"); -# define CHECK_LIST_COUNT(list, cnt) \ - if ((count = virPCIDeviceListCount(list)) != cnt) { \ - virReportError(VIR_ERR_INTERNAL_ERROR, \ - "Unexpected count of items in " #list ": %d, " \ - "expecting %zu", count, (size_t) cnt); \ - goto cleanup; \ - } +# define CHECK_LIST_COUNT(list, cnt) \ + do { \ + int count; \ + if ((count = virPCIDeviceListCount(list)) != cnt) { \ + virReportError(VIR_ERR_INTERNAL_ERROR, \ + "Unexpected count of items in " #list ": %d, " \ + "expecting %zu", count, (size_t) cnt); \ + goto cleanup; \ + } \ + } while (0) # define TEST_STATE_DIR abs_builddir "/hostdevmgr" static const char *drv_name = "test_driver"; @@ -161,7 +164,7 @@ testVirHostdevPreparePCIHostdevs_unmanaged(const void *opaque ATTRIBUTE_UNUSED) { int ret = -1; size_t i; - int count, active_count, inactive_count; + int active_count, inactive_count; for (i = 0; i < nhostdevs; i++) hostdevs[i]->managed = false; @@ -220,7 +223,7 @@ testVirHostdevReAttachPCIHostdevs_unmanaged(const void *opaque ATTRIBUTE_UNUSED) { int ret = -1; size_t i; - int count, active_count, inactive_count; + int active_count, inactive_count; for (i = 0; i < nhostdevs; i++) { if (hostdevs[i]->managed != false) { @@ -254,7 +257,7 @@ testVirHostdevPreparePCIHostdevs_managed(const void *opaque ATTRIBUTE_UNUSED) { int ret = -1; size_t i; - int count, active_count; + int active_count; for (i = 0; i < nhostdevs; i++) hostdevs[i]->managed = true; @@ -300,7 +303,7 @@ testVirHostdevReAttachPCIHostdevs_managed(const void *opaque ATTRIBUTE_UNUSED) { int ret = -1; size_t i; - int count, active_count; + int active_count; for (i = 0; i < nhostdevs; i++) { if (hostdevs[i]->managed != true) { @@ -332,7 +335,7 @@ testVirHostdevDetachPCINodeDevice(const void *opaque ATTRIBUTE_UNUSED) { int ret = -1; size_t i; - int count, inactive_count; + int inactive_count; for (i = 0; i < nhostdevs; i++) { inactive_count = virPCIDeviceListCount(mgr->inactivePCIHostdevs); @@ -369,7 +372,7 @@ testVirHostdevReAttachPCINodeDevice(const void *opaque ATTRIBUTE_UNUSED) { int ret = -1; size_t i; - int count, inactive_count; + int inactive_count; for (i = 0; i < nhostdevs; i++) { inactive_count = virPCIDeviceListCount(mgr->inactivePCIHostdevs); @@ -389,7 +392,7 @@ static int testVirHostdevUpdateActivePCIHostdevs(const void *opaque ATTRIBUTE_UNUSED) { int ret = -1; - int count, active_count; + int active_count; active_count = virPCIDeviceListCount(mgr->activePCIHostdevs); -- 2.5.0
On 03/07/2016 12:24 PM, Andrea Bolognani wrote:
This variable is only used internally by the macro, so it's better to move its definition inside the macro as well. --- tests/virhostdevtest.c | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-)
diff --git a/tests/virhostdevtest.c b/tests/virhostdevtest.c index febb2c9..77b119b 100644 --- a/tests/virhostdevtest.c +++ b/tests/virhostdevtest.c @@ -37,13 +37,16 @@
VIR_LOG_INIT("tests.hostdevtest");
-# define CHECK_LIST_COUNT(list, cnt) \ - if ((count = virPCIDeviceListCount(list)) != cnt) { \ - virReportError(VIR_ERR_INTERNAL_ERROR, \ - "Unexpected count of items in " #list ": %d, " \ - "expecting %zu", count, (size_t) cnt); \ - goto cleanup; \ - } +# define CHECK_LIST_COUNT(list, cnt) \ + do { \ + int count; \ + if ((count = virPCIDeviceListCount(list)) != cnt) { \ + virReportError(VIR_ERR_INTERNAL_ERROR, \ + "Unexpected count of items in " #list ": %d, " \ + "expecting %zu", count, (size_t) cnt); \ + goto cleanup; \ + } \ + } while (0)
The only suggestion I would have is to make "count" something less common, so that you're less likely to end up causing a compiler warning later if someone adds a variable called "count" to a function that uses this macro. Otherwise ACK.
On Mon, 2016-03-07 at 13:18 -0500, Laine Stump wrote:
+# define CHECK_LIST_COUNT(list, cnt) \ + do { \ + int count; \ + if ((count = virPCIDeviceListCount(list)) != cnt) { \ + virReportError(VIR_ERR_INTERNAL_ERROR, \ + "Unexpected count of items in " #list ": %d, " \ + "expecting %zu", count, (size_t) cnt); \ + goto cleanup; \ + } \ + } while (0) The only suggestion I would have is to make "count" something less common, so that you're less likely to end up causing a compiler warning later if someone adds a variable called "count" to a function that uses this macro.
Otherwise ACK.
I changed it to 'actualCount' before pushing, and explained the reason for the change in the commit message. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
virPCIDeviceListCount()'s return type is size_t, so variables that store its return value should be of that type. --- tests/virhostdevtest.c | 24 +++++++++--------------- 1 file changed, 9 insertions(+), 15 deletions(-) diff --git a/tests/virhostdevtest.c b/tests/virhostdevtest.c index 77b119b..c1321b1 100644 --- a/tests/virhostdevtest.c +++ b/tests/virhostdevtest.c @@ -39,10 +39,10 @@ VIR_LOG_INIT("tests.hostdevtest"); # define CHECK_LIST_COUNT(list, cnt) \ do { \ - int count; \ + size_t count; \ if ((count = virPCIDeviceListCount(list)) != cnt) { \ virReportError(VIR_ERR_INTERNAL_ERROR, \ - "Unexpected count of items in " #list ": %d, " \ + "Unexpected count of items in " #list ": %zu, " \ "expecting %zu", count, (size_t) cnt); \ goto cleanup; \ } \ @@ -163,8 +163,7 @@ static int testVirHostdevPreparePCIHostdevs_unmanaged(const void *opaque ATTRIBUTE_UNUSED) { int ret = -1; - size_t i; - int active_count, inactive_count; + size_t active_count, inactive_count, i; for (i = 0; i < nhostdevs; i++) hostdevs[i]->managed = false; @@ -222,8 +221,7 @@ static int testVirHostdevReAttachPCIHostdevs_unmanaged(const void *opaque ATTRIBUTE_UNUSED) { int ret = -1; - size_t i; - int active_count, inactive_count; + size_t active_count, inactive_count, i; for (i = 0; i < nhostdevs; i++) { if (hostdevs[i]->managed != false) { @@ -256,8 +254,7 @@ static int testVirHostdevPreparePCIHostdevs_managed(const void *opaque ATTRIBUTE_UNUSED) { int ret = -1; - size_t i; - int active_count; + size_t active_count, i; for (i = 0; i < nhostdevs; i++) hostdevs[i]->managed = true; @@ -302,8 +299,7 @@ static int testVirHostdevReAttachPCIHostdevs_managed(const void *opaque ATTRIBUTE_UNUSED) { int ret = -1; - size_t i; - int active_count; + size_t active_count, i; for (i = 0; i < nhostdevs; i++) { if (hostdevs[i]->managed != true) { @@ -334,8 +330,7 @@ static int testVirHostdevDetachPCINodeDevice(const void *opaque ATTRIBUTE_UNUSED) { int ret = -1; - size_t i; - int inactive_count; + size_t inactive_count, i; for (i = 0; i < nhostdevs; i++) { inactive_count = virPCIDeviceListCount(mgr->inactivePCIHostdevs); @@ -371,8 +366,7 @@ static int testVirHostdevReAttachPCINodeDevice(const void *opaque ATTRIBUTE_UNUSED) { int ret = -1; - size_t i; - int inactive_count; + size_t inactive_count, i; for (i = 0; i < nhostdevs; i++) { inactive_count = virPCIDeviceListCount(mgr->inactivePCIHostdevs); @@ -392,7 +386,7 @@ static int testVirHostdevUpdateActivePCIHostdevs(const void *opaque ATTRIBUTE_UNUSED) { int ret = -1; - int active_count; + size_t active_count; active_count = virPCIDeviceListCount(mgr->activePCIHostdevs); -- 2.5.0
On 03/07/2016 12:24 PM, Andrea Bolognani wrote:
virPCIDeviceListCount()'s return type is size_t, so variables that store its return value should be of that type. --- tests/virhostdevtest.c | 24 +++++++++--------------- 1 file changed, 9 insertions(+), 15 deletions(-)
ACK.
Always call CHECK_LIST_COUNT() to check the size of both the active and inactive devices list. --- tests/virhostdevtest.c | 34 ++++++++++++++++++++++++++++------ 1 file changed, 28 insertions(+), 6 deletions(-) diff --git a/tests/virhostdevtest.c b/tests/virhostdevtest.c index c1321b1..8dc92f6 100644 --- a/tests/virhostdevtest.c +++ b/tests/virhostdevtest.c @@ -177,6 +177,7 @@ testVirHostdevPreparePCIHostdevs_unmanaged(const void *opaque ATTRIBUTE_UNUSED) NULL, 0, 0) < 0) goto cleanup; CHECK_LIST_COUNT(mgr->activePCIHostdevs, active_count); + CHECK_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); /* Test unmanaged hostdevs */ VIR_DEBUG("Test >=1 unmanaged hostdevs"); @@ -236,6 +237,7 @@ testVirHostdevReAttachPCIHostdevs_unmanaged(const void *opaque ATTRIBUTE_UNUSED) VIR_DEBUG("Test 0 hostdevs"); virHostdevReAttachPCIDevices(mgr, drv_name, dom_name, NULL, 0, NULL); CHECK_LIST_COUNT(mgr->activePCIHostdevs, active_count); + CHECK_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); VIR_DEBUG("Test >=1 unmanaged hostdevs"); virHostdevReAttachPCIDevices(mgr, drv_name, dom_name, @@ -254,12 +256,13 @@ static int testVirHostdevPreparePCIHostdevs_managed(const void *opaque ATTRIBUTE_UNUSED) { int ret = -1; - size_t active_count, i; + size_t active_count, inactive_count, i; for (i = 0; i < nhostdevs; i++) hostdevs[i]->managed = true; active_count = virPCIDeviceListCount(mgr->activePCIHostdevs); + inactive_count = virPCIDeviceListCount(mgr->inactivePCIHostdevs); /* Test normal functionality */ VIR_DEBUG("Test >=1 hostdevs"); @@ -267,26 +270,31 @@ testVirHostdevPreparePCIHostdevs_managed(const void *opaque ATTRIBUTE_UNUSED) hostdevs, nhostdevs, 0) < 0) goto cleanup; CHECK_LIST_COUNT(mgr->activePCIHostdevs, active_count + nhostdevs); + CHECK_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); /* Test conflict */ active_count = virPCIDeviceListCount(mgr->activePCIHostdevs); + inactive_count = virPCIDeviceListCount(mgr->inactivePCIHostdevs); VIR_DEBUG("Test: prepare same hostdevs for same driver/domain again"); if (!virHostdevPreparePCIDevices(mgr, drv_name, dom_name, uuid, &hostdevs[0], 1, 0)) goto cleanup; CHECK_LIST_COUNT(mgr->activePCIHostdevs, active_count); + CHECK_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); VIR_DEBUG("Test: prepare same hostdevs for same driver, diff domain again"); if (!virHostdevPreparePCIDevices(mgr, drv_name, "test_domain1", uuid, &hostdevs[1], 1, 0)) goto cleanup; CHECK_LIST_COUNT(mgr->activePCIHostdevs, active_count); + CHECK_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); VIR_DEBUG("Test: prepare same hostdevs for diff driver/domain again"); if (!virHostdevPreparePCIDevices(mgr, "test_driver1", dom_name, uuid, &hostdevs[2], 1, 0)) goto cleanup; CHECK_LIST_COUNT(mgr->activePCIHostdevs, active_count); + CHECK_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); ret = 0; @@ -299,7 +307,7 @@ static int testVirHostdevReAttachPCIHostdevs_managed(const void *opaque ATTRIBUTE_UNUSED) { int ret = -1; - size_t active_count, i; + size_t active_count, inactive_count, i; for (i = 0; i < nhostdevs; i++) { if (hostdevs[i]->managed != true) { @@ -309,15 +317,18 @@ testVirHostdevReAttachPCIHostdevs_managed(const void *opaque ATTRIBUTE_UNUSED) } active_count = virPCIDeviceListCount(mgr->activePCIHostdevs); + inactive_count = virPCIDeviceListCount(mgr->inactivePCIHostdevs); VIR_DEBUG("Test 0 hostdevs"); virHostdevReAttachPCIDevices(mgr, drv_name, dom_name, NULL, 0, NULL); CHECK_LIST_COUNT(mgr->activePCIHostdevs, active_count); + CHECK_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); VIR_DEBUG("Test >=1 hostdevs"); virHostdevReAttachPCIDevices(mgr, drv_name, dom_name, hostdevs, nhostdevs, NULL); CHECK_LIST_COUNT(mgr->activePCIHostdevs, active_count - nhostdevs); + CHECK_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); ret = 0; @@ -330,12 +341,14 @@ static int testVirHostdevDetachPCINodeDevice(const void *opaque ATTRIBUTE_UNUSED) { int ret = -1; - size_t inactive_count, i; + size_t active_count, inactive_count, i; for (i = 0; i < nhostdevs; i++) { + active_count = virPCIDeviceListCount(mgr->activePCIHostdevs); inactive_count = virPCIDeviceListCount(mgr->inactivePCIHostdevs); if (virHostdevPCINodeDeviceDetach(mgr, dev[i]) < 0) goto cleanup; + CHECK_LIST_COUNT(mgr->activePCIHostdevs, active_count); CHECK_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count + 1); } @@ -348,11 +361,15 @@ static int testVirHostdevResetPCINodeDevice(const void *opaque ATTRIBUTE_UNUSED) { int ret = -1; - size_t i; + size_t active_count, inactive_count, i; for (i = 0; i < nhostdevs; i++) { + active_count = virPCIDeviceListCount(mgr->activePCIHostdevs); + inactive_count = virPCIDeviceListCount(mgr->inactivePCIHostdevs); if (virHostdevPCINodeDeviceReset(mgr, dev[i]) < 0) goto cleanup; + CHECK_LIST_COUNT(mgr->activePCIHostdevs, active_count); + CHECK_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); } ret = 0; @@ -366,12 +383,14 @@ static int testVirHostdevReAttachPCINodeDevice(const void *opaque ATTRIBUTE_UNUSED) { int ret = -1; - size_t inactive_count, i; + size_t active_count, inactive_count, i; for (i = 0; i < nhostdevs; i++) { + active_count = virPCIDeviceListCount(mgr->activePCIHostdevs); inactive_count = virPCIDeviceListCount(mgr->inactivePCIHostdevs); if (virHostdevPCINodeDeviceReAttach(mgr, dev[i]) < 0) goto cleanup; + CHECK_LIST_COUNT(mgr->activePCIHostdevs, active_count); CHECK_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count - 1); } @@ -386,21 +405,24 @@ static int testVirHostdevUpdateActivePCIHostdevs(const void *opaque ATTRIBUTE_UNUSED) { int ret = -1; - size_t active_count; + size_t active_count, inactive_count; active_count = virPCIDeviceListCount(mgr->activePCIHostdevs); + inactive_count = virPCIDeviceListCount(mgr->inactivePCIHostdevs); VIR_DEBUG("Test 0 hostdevs"); if (virHostdevUpdateActivePCIDevices(mgr, NULL, 0, drv_name, dom_name) < 0) goto cleanup; CHECK_LIST_COUNT(mgr->activePCIHostdevs, active_count); + CHECK_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); VIR_DEBUG("Test >=1 hostdevs"); if (virHostdevUpdateActivePCIDevices(mgr, hostdevs, nhostdevs, drv_name, dom_name) < 0) goto cleanup; CHECK_LIST_COUNT(mgr->activePCIHostdevs, active_count + nhostdevs); + CHECK_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); ret = 0; -- 2.5.0
On 03/07/2016 12:24 PM, Andrea Bolognani wrote:
Always call CHECK_LIST_COUNT() to check the size of both the active and inactive devices list. --- tests/virhostdevtest.c | 34 ++++++++++++++++++++++++++++------ 1 file changed, 28 insertions(+), 6 deletions(-)
ACK.
Instead of considering each single step its own test case, create high level test cases that reproduce a certain scenario. --- tests/virhostdevtest.c | 119 ++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 97 insertions(+), 22 deletions(-) diff --git a/tests/virhostdevtest.c b/tests/virhostdevtest.c index 8dc92f6..c0ab044 100644 --- a/tests/virhostdevtest.c +++ b/tests/virhostdevtest.c @@ -160,7 +160,7 @@ virHostdevHostSupportsPassthroughKVM(void) # endif static int -testVirHostdevPreparePCIHostdevs_unmanaged(const void *opaque ATTRIBUTE_UNUSED) +testVirHostdevPreparePCIHostdevs_unmanaged(void) { int ret = -1; size_t active_count, inactive_count, i; @@ -219,7 +219,7 @@ testVirHostdevPreparePCIHostdevs_unmanaged(const void *opaque ATTRIBUTE_UNUSED) } static int -testVirHostdevReAttachPCIHostdevs_unmanaged(const void *opaque ATTRIBUTE_UNUSED) +testVirHostdevReAttachPCIHostdevs_unmanaged(void) { int ret = -1; size_t active_count, inactive_count, i; @@ -253,7 +253,7 @@ testVirHostdevReAttachPCIHostdevs_unmanaged(const void *opaque ATTRIBUTE_UNUSED) } static int -testVirHostdevPreparePCIHostdevs_managed(const void *opaque ATTRIBUTE_UNUSED) +testVirHostdevPreparePCIHostdevs_managed(void) { int ret = -1; size_t active_count, inactive_count, i; @@ -304,7 +304,7 @@ testVirHostdevPreparePCIHostdevs_managed(const void *opaque ATTRIBUTE_UNUSED) } static int -testVirHostdevReAttachPCIHostdevs_managed(const void *opaque ATTRIBUTE_UNUSED) +testVirHostdevReAttachPCIHostdevs_managed(void) { int ret = -1; size_t active_count, inactive_count, i; @@ -338,7 +338,7 @@ testVirHostdevReAttachPCIHostdevs_managed(const void *opaque ATTRIBUTE_UNUSED) } static int -testVirHostdevDetachPCINodeDevice(const void *opaque ATTRIBUTE_UNUSED) +testVirHostdevDetachPCINodeDevice(void) { int ret = -1; size_t active_count, inactive_count, i; @@ -357,8 +357,9 @@ testVirHostdevDetachPCINodeDevice(const void *opaque ATTRIBUTE_UNUSED) cleanup: return ret; } + static int -testVirHostdevResetPCINodeDevice(const void *opaque ATTRIBUTE_UNUSED) +testVirHostdevResetPCINodeDevice(void) { int ret = -1; size_t active_count, inactive_count, i; @@ -380,7 +381,7 @@ testVirHostdevResetPCINodeDevice(const void *opaque ATTRIBUTE_UNUSED) } static int -testVirHostdevReAttachPCINodeDevice(const void *opaque ATTRIBUTE_UNUSED) +testVirHostdevReAttachPCINodeDevice(void) { int ret = -1; size_t active_count, inactive_count, i; @@ -402,7 +403,7 @@ testVirHostdevReAttachPCINodeDevice(const void *opaque ATTRIBUTE_UNUSED) } static int -testVirHostdevUpdateActivePCIHostdevs(const void *opaque ATTRIBUTE_UNUSED) +testVirHostdevUpdateActivePCIHostdevs(void) { int ret = -1; size_t active_count, inactive_count; @@ -430,6 +431,91 @@ testVirHostdevUpdateActivePCIHostdevs(const void *opaque ATTRIBUTE_UNUSED) return ret; } +/** + * testVirHostdevRoundtripUnmanaged: + * @opaque: unused + * + * Perform a roundtrip with unmanaged devices. + * + * 1. Detach devices from the host + * 2. Attach devices to the guest as unmanaged + * 3. Detach devices from the guest as unmanaged + * 4. Reattach devices to the host + */ +static int +testVirHostdevRoundtripUnmanaged(const void *opaque ATTRIBUTE_UNUSED) +{ + int ret = -1; + + if (testVirHostdevDetachPCINodeDevice() < 0) + goto out; + if (virHostdevHostSupportsPassthroughKVM()) { + if (testVirHostdevPreparePCIHostdevs_unmanaged() < 0) + goto out; + if (testVirHostdevReAttachPCIHostdevs_unmanaged() < 0) + goto out; + } + if (testVirHostdevReAttachPCINodeDevice() < 0) + goto out; + + ret = 0; + + out: + return ret; +} + +/** + * testVirHostdevRoundtripManaged: + * @opaque: unused + * + * Perform a roundtrip with managed devices. + * + * 1. Attach devices to the guest as managed + * 2. Detach devices from the guest as managed + */ +static int +testVirHostdevRoundtripManaged(const void *opaque ATTRIBUTE_UNUSED) +{ + int ret = -1; + + if (virHostdevHostSupportsPassthroughKVM()) { + if (testVirHostdevPreparePCIHostdevs_managed() < 0) + goto out; + if (testVirHostdevReAttachPCIHostdevs_managed() < 0) + goto out; + } + + ret = 0; + + out: + return ret; +} + +/** + * testVirHostdevOther: + * @opaque: unused + * + * Perform other operations on devices. + * + * 1. Reset devices + * 2. Update list of active devices + */ +static int +testVirHostdevOther(const void *opaque ATTRIBUTE_UNUSED) +{ + int ret = -1; + + if (testVirHostdevResetPCINodeDevice() < 0) + goto out; + if (testVirHostdevUpdateActivePCIHostdevs() < 0) + goto out; + + ret = 0; + + out: + return ret; +} + # define FAKEROOTDIRTEMPLATE abs_builddir "/fakerootdir-XXXXXX" static int @@ -460,20 +546,9 @@ mymain(void) if (myInit() < 0) fprintf(stderr, "Init data structures failed."); - DO_TEST(testVirHostdevDetachPCINodeDevice); - if (virHostdevHostSupportsPassthroughKVM()) { - /* following tests would check KVM support */ - DO_TEST(testVirHostdevPreparePCIHostdevs_unmanaged); - DO_TEST(testVirHostdevReAttachPCIHostdevs_unmanaged); - } - DO_TEST(testVirHostdevResetPCINodeDevice); - DO_TEST(testVirHostdevReAttachPCINodeDevice); - if (virHostdevHostSupportsPassthroughKVM()) { - /* following tests would check KVM support */ - DO_TEST(testVirHostdevPreparePCIHostdevs_managed); - DO_TEST(testVirHostdevReAttachPCIHostdevs_managed); - } - DO_TEST(testVirHostdevUpdateActivePCIHostdevs); + DO_TEST(testVirHostdevRoundtripUnmanaged); + DO_TEST(testVirHostdevRoundtripManaged); + DO_TEST(testVirHostdevOther); myCleanup(); -- 2.5.0
On 03/07/2016 12:24 PM, Andrea Bolognani wrote:
Instead of considering each single step its own test case, create high level test cases that reproduce a certain scenario. --- tests/virhostdevtest.c | 119 ++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 97 insertions(+), 22 deletions(-)
I like the idea, just have two comments: 1) would it maybe be useful to keep the individual tests, in order to make it simpler to determine which piece had broken in the case of a regression? 2) It looks like this only tests for legacy kvm (i.e. attaching/detaching pci-stub). It would be good to have it test for vfio as well (although maybe that can be saved for after we add support for using individual devices' driver_override to attach different drivers). ACK as is, the comments are food for thought.
diff --git a/tests/virhostdevtest.c b/tests/virhostdevtest.c index 8dc92f6..c0ab044 100644 --- a/tests/virhostdevtest.c +++ b/tests/virhostdevtest.c @@ -160,7 +160,7 @@ virHostdevHostSupportsPassthroughKVM(void) # endif
static int -testVirHostdevPreparePCIHostdevs_unmanaged(const void *opaque ATTRIBUTE_UNUSED) +testVirHostdevPreparePCIHostdevs_unmanaged(void) { int ret = -1; size_t active_count, inactive_count, i; @@ -219,7 +219,7 @@ testVirHostdevPreparePCIHostdevs_unmanaged(const void *opaque ATTRIBUTE_UNUSED) }
static int -testVirHostdevReAttachPCIHostdevs_unmanaged(const void *opaque ATTRIBUTE_UNUSED) +testVirHostdevReAttachPCIHostdevs_unmanaged(void) { int ret = -1; size_t active_count, inactive_count, i; @@ -253,7 +253,7 @@ testVirHostdevReAttachPCIHostdevs_unmanaged(const void *opaque ATTRIBUTE_UNUSED) }
static int -testVirHostdevPreparePCIHostdevs_managed(const void *opaque ATTRIBUTE_UNUSED) +testVirHostdevPreparePCIHostdevs_managed(void) { int ret = -1; size_t active_count, inactive_count, i; @@ -304,7 +304,7 @@ testVirHostdevPreparePCIHostdevs_managed(const void *opaque ATTRIBUTE_UNUSED) }
static int -testVirHostdevReAttachPCIHostdevs_managed(const void *opaque ATTRIBUTE_UNUSED) +testVirHostdevReAttachPCIHostdevs_managed(void) { int ret = -1; size_t active_count, inactive_count, i; @@ -338,7 +338,7 @@ testVirHostdevReAttachPCIHostdevs_managed(const void *opaque ATTRIBUTE_UNUSED) }
static int -testVirHostdevDetachPCINodeDevice(const void *opaque ATTRIBUTE_UNUSED) +testVirHostdevDetachPCINodeDevice(void) { int ret = -1; size_t active_count, inactive_count, i; @@ -357,8 +357,9 @@ testVirHostdevDetachPCINodeDevice(const void *opaque ATTRIBUTE_UNUSED) cleanup: return ret; } + static int -testVirHostdevResetPCINodeDevice(const void *opaque ATTRIBUTE_UNUSED) +testVirHostdevResetPCINodeDevice(void) { int ret = -1; size_t active_count, inactive_count, i; @@ -380,7 +381,7 @@ testVirHostdevResetPCINodeDevice(const void *opaque ATTRIBUTE_UNUSED) }
static int -testVirHostdevReAttachPCINodeDevice(const void *opaque ATTRIBUTE_UNUSED) +testVirHostdevReAttachPCINodeDevice(void) { int ret = -1; size_t active_count, inactive_count, i; @@ -402,7 +403,7 @@ testVirHostdevReAttachPCINodeDevice(const void *opaque ATTRIBUTE_UNUSED) }
static int -testVirHostdevUpdateActivePCIHostdevs(const void *opaque ATTRIBUTE_UNUSED) +testVirHostdevUpdateActivePCIHostdevs(void) { int ret = -1; size_t active_count, inactive_count; @@ -430,6 +431,91 @@ testVirHostdevUpdateActivePCIHostdevs(const void *opaque ATTRIBUTE_UNUSED) return ret; }
+/** + * testVirHostdevRoundtripUnmanaged: + * @opaque: unused + * + * Perform a roundtrip with unmanaged devices. + * + * 1. Detach devices from the host + * 2. Attach devices to the guest as unmanaged + * 3. Detach devices from the guest as unmanaged + * 4. Reattach devices to the host + */ +static int +testVirHostdevRoundtripUnmanaged(const void *opaque ATTRIBUTE_UNUSED) +{ + int ret = -1; + + if (testVirHostdevDetachPCINodeDevice() < 0) + goto out; + if (virHostdevHostSupportsPassthroughKVM()) { + if (testVirHostdevPreparePCIHostdevs_unmanaged() < 0) + goto out; + if (testVirHostdevReAttachPCIHostdevs_unmanaged() < 0) + goto out; + } + if (testVirHostdevReAttachPCINodeDevice() < 0) + goto out; + + ret = 0; + + out: + return ret; +} + +/** + * testVirHostdevRoundtripManaged: + * @opaque: unused + * + * Perform a roundtrip with managed devices. + * + * 1. Attach devices to the guest as managed + * 2. Detach devices from the guest as managed + */ +static int +testVirHostdevRoundtripManaged(const void *opaque ATTRIBUTE_UNUSED) +{ + int ret = -1; + + if (virHostdevHostSupportsPassthroughKVM()) { + if (testVirHostdevPreparePCIHostdevs_managed() < 0) + goto out; + if (testVirHostdevReAttachPCIHostdevs_managed() < 0) + goto out; + } + + ret = 0; + + out: + return ret; +} + +/** + * testVirHostdevOther: + * @opaque: unused + * + * Perform other operations on devices. + * + * 1. Reset devices + * 2. Update list of active devices + */ +static int +testVirHostdevOther(const void *opaque ATTRIBUTE_UNUSED) +{ + int ret = -1; + + if (testVirHostdevResetPCINodeDevice() < 0) + goto out; + if (testVirHostdevUpdateActivePCIHostdevs() < 0) + goto out; + + ret = 0; + + out: + return ret; +} + # define FAKEROOTDIRTEMPLATE abs_builddir "/fakerootdir-XXXXXX"
static int @@ -460,20 +546,9 @@ mymain(void) if (myInit() < 0) fprintf(stderr, "Init data structures failed.");
- DO_TEST(testVirHostdevDetachPCINodeDevice); - if (virHostdevHostSupportsPassthroughKVM()) { - /* following tests would check KVM support */ - DO_TEST(testVirHostdevPreparePCIHostdevs_unmanaged); - DO_TEST(testVirHostdevReAttachPCIHostdevs_unmanaged); - } - DO_TEST(testVirHostdevResetPCINodeDevice); - DO_TEST(testVirHostdevReAttachPCINodeDevice); - if (virHostdevHostSupportsPassthroughKVM()) { - /* following tests would check KVM support */ - DO_TEST(testVirHostdevPreparePCIHostdevs_managed); - DO_TEST(testVirHostdevReAttachPCIHostdevs_managed); - } - DO_TEST(testVirHostdevUpdateActivePCIHostdevs); + DO_TEST(testVirHostdevRoundtripUnmanaged); + DO_TEST(testVirHostdevRoundtripManaged); + DO_TEST(testVirHostdevOther);
myCleanup();
On Mon, 2016-03-07 at 21:57 -0500, Laine Stump wrote: > On 03/07/2016 12:24 PM, Andrea Bolognani wrote: > > > > Instead of considering each single step its own test case, create > > high level test cases that reproduce a certain scenario. > > --- > > tests/virhostdevtest.c | 119 ++++++++++++++++++++++++++++++++++++++++--------- > > 1 file changed, 97 insertions(+), 22 deletions(-) > I like the idea, just have two comments: > > 1) would it maybe be useful to keep the individual tests, in order to > make it simpler to determine which piece had broken in the case of a > regression? If some test case fails you're probably going to be using VIR_TEST_DEBUG, gdb or a combination of the two to figure out what went wrong, so I'm not convinced testing the functions separately would add much value. > 2) It looks like this only tests for legacy kvm (i.e. > attaching/detaching pci-stub). It would be good to have it test for vfio > as well (although maybe that can be saved for after we add support for > using individual devices' driver_override to attach different drivers). Sure, adding more test cases is always good :) I'm not sure what this driver_override you refer to is supposed to be though, care to expand on that? > ACK as is, the comments are food for thought. Pushed along with the rest. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
Replace the term "loop" with the more generic "step". This allows us to be more flexible and eg. have a step that consists in a single function call. Don't include the number of steps in the first comment of the function, so that we can add or remove steps without having to worry about keeping that comment in sync. For the same reason, remove the summary contained in that comment. Clean up some weird vertical spacing while we're at it. --- src/util/virhostdev.c | 58 +++++++++++++++++++++++---------------------------- 1 file changed, 26 insertions(+), 32 deletions(-) diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index 2db0da0..adc4eec 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -546,18 +546,18 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, if (!(pcidevs = virHostdevGetPCIHostDeviceList(hostdevs, nhostdevs))) goto cleanup; - /* We have to use 9 loops here. *All* devices must - * be detached before we reset any of them, because - * in some cases you have to reset the whole PCI, - * which impacts all devices on it. Also, all devices - * must be reset before being marked as active. - */ - - /* Loop 1: validate that non-managed device isn't in use, eg + /* Detaching devices from the host involves several steps; each + * of them is described at length below. + * + * All devices must be detached before we reset any of them, + * because in some cases you have to reset the whole PCI, which + * impacts all devices on it. Also, all devices must be reset + * before being marked as active */ + + /* Step 1: validate that non-managed device isn't in use, eg * by checking that device is either un-bound, or bound * to pci-stub.ko */ - for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); bool strict_acs_check = !!(flags & VIR_HOSTDEV_STRICT_ACS_CHECK); @@ -588,7 +588,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, } } - /* Loop 2: detach managed devices (i.e. bind to appropriate stub driver) */ + /* Step 2: detach managed devices (i.e. bind to appropriate stub driver) */ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); @@ -608,7 +608,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, /* At this point, all devices are attached to the stub driver and have * been marked as inactive */ - /* Loop 3: Now that all the PCI hostdevs have been detached, we + /* Step 3: Now that all the PCI hostdevs have been detached, we * can safely reset them */ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); @@ -619,7 +619,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, goto reattachdevs; } - /* Loop 4: For SRIOV network devices, Now that we have detached the + /* Step 4: For SRIOV network devices, Now that we have detached the * the network device, set the netdev config */ for (i = 0; i < nhostdevs; i++) { virDomainHostdevDefPtr hostdev = hostdevs[i]; @@ -632,7 +632,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, last_processed_hostdev_vf = i; } - /* Loop 5: Now mark all the devices as active */ + /* Step 5: Now mark all the devices as active */ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); @@ -642,7 +642,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, goto inactivedevs; } - /* Loop 6: Now remove the devices from inactive list. */ + /* Step 6: Now remove the devices from inactive list. */ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); @@ -651,7 +651,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, virPCIDeviceListDel(hostdev_mgr->inactivePCIHostdevs, dev); } - /* Loop 7: Now set the used_by_domain of the device in + /* Step 7: Now set the used_by_domain of the device in * activePCIHostdevs as domain name. */ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { @@ -666,7 +666,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, virPCIDeviceSetUsedBy(activeDev, drv_name, dom_name); } - /* Loop 8: Now set the original states for hostdev def */ + /* Step 8: Now set the original states for hostdev def */ for (i = 0; i < nhostdevs; i++) { virPCIDevicePtr dev; virPCIDevicePtr pcidev; @@ -681,10 +681,9 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, dev = virPCIDeviceNew(pcisrc->addr.domain, pcisrc->addr.bus, pcisrc->addr.slot, pcisrc->addr.function); - /* original states "unbind_from_stub", "remove_slot", - * "reprobe" were already set by pciDettachDevice in - * loop 2. - */ + /* Appropriate values for the unbind_from_stub, remove_slot + * and reprobe properties of the device were set earlier + * by virPCIDeviceDetach() */ VIR_DEBUG("Saving network configuration of PCI device %s", virPCIDeviceGetName(dev)); if ((pcidev = virPCIDeviceListFind(pcidevs, dev))) { @@ -699,7 +698,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, virPCIDeviceFree(dev); } - /* Loop 9: Now steal all the devices from pcidevs */ + /* Step 9: Now steal all the devices from pcidevs */ while (virPCIDeviceListCount(pcidevs) > 0) virPCIDeviceListStealIndex(pcidevs, 0); @@ -819,14 +818,10 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr, goto cleanup; } - /* Loop through the assigned devices 4 times: 1) delete them all from - * activePCIHostdevs, 2) restore network config of SRIOV netdevs, 3) Do a - * PCI reset on each device, 4) reattach the devices to their host drivers - * (managed) or add them to inactivePCIHostdevs (!managed). - */ + /* Reattaching devices to the host involves several steps; each + * of them is described at length below */ - /* - * Loop 1: verify that each device in the hostdevs list really was in use + /* Step 1: verify that each device in the hostdevs list really was in use * by this domain, and remove them all from the activePCIHostdevs list. */ i = 0; @@ -856,8 +851,7 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr, * pcidevs, but has been removed from activePCIHostdevs. */ - /* - * Loop 2: restore original network config of hostdevs that used + /* Step 2: restore original network config of hostdevs that used * <interface type='hostdev'> */ for (i = 0; i < nhostdevs; i++) { @@ -880,7 +874,7 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr, } } - /* Loop 3: perform a PCI Reset on all devices */ + /* Step 3: perform a PCI Reset on all devices */ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); @@ -894,7 +888,7 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr, } } - /* Loop 4: reattach devices to their host drivers (if managed) or place + /* Step 4: reattach devices to their host drivers (if managed) or place * them on the inactive list (if not managed) */ while (virPCIDeviceListCount(pcidevs) > 0) { -- 2.5.0
On 03/07/2016 12:24 PM, Andrea Bolognani wrote:
Replace the term "loop" with the more generic "step". This allows us to be more flexible and eg. have a step that consists in a single function call.
Don't include the number of steps in the first comment of the function, so that we can add or remove steps without having to worry about keeping that comment in sync.
For the same reason, remove the summary contained in that comment.
Clean up some weird vertical spacing while we're at it. --- src/util/virhostdev.c | 58 +++++++++++++++++++++++---------------------------- 1 file changed, 26 insertions(+), 32 deletions(-)
ACK.
--- src/util/virhostdev.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index adc4eec..5be61cd 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -727,9 +727,6 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); if (virPCIDeviceGetManaged(dev)) { - /* NB: This doesn't actually re-bind to original driver, just - * unbinds from the stub driver - */ VIR_DEBUG("Reattaching managed PCI device %s", virPCIDeviceGetName(dev)); ignore_value(virPCIDeviceReattach(dev, -- 2.5.0
On 03/07/2016 12:24 PM, Andrea Bolognani wrote:
--- src/util/virhostdev.c | 3 --- 1 file changed, 3 deletions(-)
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index adc4eec..5be61cd 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -727,9 +727,6 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i);
if (virPCIDeviceGetManaged(dev)) { - /* NB: This doesn't actually re-bind to original driver, just - * unbinds from the stub driver - */ VIR_DEBUG("Reattaching managed PCI device %s", virPCIDeviceGetName(dev)); ignore_value(virPCIDeviceReattach(dev,
ACK.
On Mon, 2016-03-07 at 22:02 -0500, Laine Stump wrote:
On 03/07/2016 12:24 PM, Andrea Bolognani wrote:
--- src/util/virhostdev.c | 3 --- 1 file changed, 3 deletions(-)
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index adc4eec..5be61cd 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -727,9 +727,6 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); if (virPCIDeviceGetManaged(dev)) { - /* NB: This doesn't actually re-bind to original driver, just - * unbinds from the stub driver - */ VIR_DEBUG("Reattaching managed PCI device %s", virPCIDeviceGetName(dev)); ignore_value(virPCIDeviceReattach(dev,
ACK.
The commit message was supposed to be a placeholder, so I reworked it a bit before pushing. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
--- src/util/virhostdev.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index 5be61cd..48a44bc 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -833,9 +833,10 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr, virPCIDeviceGetUsedBy(activeDev, &usedby_drvname, &usedby_domname); if (STRNEQ_NULLABLE(drv_name, usedby_drvname) || STRNEQ_NULLABLE(dom_name, usedby_domname)) { - virPCIDeviceListDel(pcidevs, dev); - continue; - } + + virPCIDeviceListDel(pcidevs, dev); + continue; + } } VIR_DEBUG("Removing PCI device %s from active list", -- 2.5.0
On 03/07/2016 12:24 PM, Andrea Bolognani wrote:
--- src/util/virhostdev.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index 5be61cd..48a44bc 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -833,9 +833,10 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr, virPCIDeviceGetUsedBy(activeDev, &usedby_drvname, &usedby_domname); if (STRNEQ_NULLABLE(drv_name, usedby_drvname) || STRNEQ_NULLABLE(dom_name, usedby_domname)) { - virPCIDeviceListDel(pcidevs, dev); - continue; - } + + virPCIDeviceListDel(pcidevs, dev); + continue; + } }
VIR_DEBUG("Removing PCI device %s from active list",
ACK :-)
NULL checks are performed implicitly in the rest of the module, including other allocations in the very same function. --- src/util/virhostdev.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index 48a44bc..098207e 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -142,16 +142,16 @@ virHostdevManagerNew(void) if (!(hostdevMgr = virObjectNew(virHostdevManagerClass))) return NULL; - if ((hostdevMgr->activePCIHostdevs = virPCIDeviceListNew()) == NULL) + if (!(hostdevMgr->activePCIHostdevs = virPCIDeviceListNew())) goto error; - if ((hostdevMgr->activeUSBHostdevs = virUSBDeviceListNew()) == NULL) + if (!(hostdevMgr->activeUSBHostdevs = virUSBDeviceListNew())) goto error; - if ((hostdevMgr->inactivePCIHostdevs = virPCIDeviceListNew()) == NULL) + if (!(hostdevMgr->inactivePCIHostdevs = virPCIDeviceListNew())) goto error; - if ((hostdevMgr->activeSCSIHostdevs = virSCSIDeviceListNew()) == NULL) + if (!(hostdevMgr->activeSCSIHostdevs = virSCSIDeviceListNew())) goto error; if (privileged) { -- 2.5.0
On 03/07/2016 12:24 PM, Andrea Bolognani wrote:
NULL checks are performed implicitly in the rest of the module, including other allocations in the very same function. --- src/util/virhostdev.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
ACK.
On Mon, 2016-03-07 at 22:03 -0500, Laine Stump wrote:
On 03/07/2016 12:24 PM, Andrea Bolognani wrote:
NULL checks are performed implicitly in the rest of the module, including other allocations in the very same function. --- src/util/virhostdev.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
ACK.
Series pushed until here, thanks for the review so far :) -- Andrea Bolognani Software Engineer - Virtualization Team
If 'last_processed_hostdev_vf != -1' is false then, since the loop counter 'i' starts at 0, 'i <= last_processed_hostdev_vf' can't possibly be true and the loop body will never be executed. Hence, the first check is completely redundant and can be safely removed. --- src/util/virhostdev.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index 098207e..f08502b 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -718,8 +718,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, } resetvfnetconfig: - for (i = 0; - last_processed_hostdev_vf != -1 && i <= last_processed_hostdev_vf; i++) + for (i = 0; i <= last_processed_hostdev_vf; i++) virHostdevNetConfigRestore(hostdevs[i], hostdev_mgr->stateDir, NULL); reattachdevs: -- 2.5.0
On 03/07/2016 12:24 PM, Andrea Bolognani wrote:
If 'last_processed_hostdev_vf != -1' is false then, since the loop counter 'i' starts at 0, 'i <= last_processed_hostdev_vf' can't possibly be true and the loop body will never be executed.
Hence, the first check is completely redundant and can be safely removed. --- src/util/virhostdev.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
Premise understood; however, Coverity has an issue...
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index 098207e..f08502b 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c
Way back here: 507 virPCIDeviceListPtr pcidevs = NULL; (1) Event var_tested_neg: Assigning: "last_processed_hostdev_vf" = a negative value. Also see events: [negative_returns] 508 int last_processed_hostdev_vf = -1; Eventually we enter this loop: for (i = 0; i < nhostdevs; i++) { virDomainHostdevDefPtr hostdev = hostdevs[i]; if (!virHostdevIsPCINetDevice(hostdev)) continue; if (virHostdevNetConfigReplace(hostdev, uuid, mgr->stateDir) < 0) { goto resetvfnetconfig; } last_processed_hostdev_vf = i; } If for some reason we "continue" (or not) and eventually "goto resetvfnetconfig;" before ever setting last_processed_hostdev_vf, then we get to the goto.,..
@@ -718,8 +718,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, }
resetvfnetconfig: - for (i = 0; - last_processed_hostdev_vf != -1 && i <= last_processed_hostdev_vf; i++)
and last_processed_hostdev_vf still == -1 So that check needs to be there - perhaps just add an: if (last_processed_hostdev_vf > -1) { } John
+ for (i = 0; i <= last_processed_hostdev_vf; i++) virHostdevNetConfigRestore(hostdevs[i], hostdev_mgr->stateDir, NULL);
reattachdevs:
On Thu, 2016-03-10 at 12:01 -0500, John Ferlan wrote:
On 03/07/2016 12:24 PM, Andrea Bolognani wrote:
If 'last_processed_hostdev_vf != -1' is false then, since the loop counter 'i' starts at 0, 'i <= last_processed_hostdev_vf' can't possibly be true and the loop body will never be executed. Hence, the first check is completely redundant and can be safely removed. --- src/util/virhostdev.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) Premise understood; however, Coverity has an issue...
Well, that's a first :P
Way back here: 507 virPCIDeviceListPtr pcidevs = NULL; (1) Event var_tested_neg: Assigning: "last_processed_hostdev_vf" = a negative value. Also see events: [negative_returns] 508 int last_processed_hostdev_vf = -1; Eventually we enter this loop: for (i = 0; i < nhostdevs; i++) { virDomainHostdevDefPtr hostdev = hostdevs[i]; if (!virHostdevIsPCINetDevice(hostdev)) continue; if (virHostdevNetConfigReplace(hostdev, uuid, mgr->stateDir) < 0) { goto resetvfnetconfig; } last_processed_hostdev_vf = i; } If for some reason we "continue" (or not) and eventually "goto resetvfnetconfig;" before ever setting last_processed_hostdev_vf, then we get to the goto.,..
resetvfnetconfig: - for (i = 0; - last_processed_hostdev_vf != -1 && i <= last_processed_hostdev_vf; i++) and last_processed_hostdev_vf still == -1 So that check needs to be there - perhaps just add an: if (last_processed_hostdev_vf > -1) { }
I fail to see how this is a problem: if last_processed_hostdev_vf has never been assigned a value after being declared, then the rewritten loop would be equivalent to for (i = 0; i <= -1; i++) { ... } which means the loop body will never be executed, just as expected. Am I missing something? Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
On 03/10/2016 12:21 PM, Andrea Bolognani wrote:
On Thu, 2016-03-10 at 12:01 -0500, John Ferlan wrote:
On 03/07/2016 12:24 PM, Andrea Bolognani wrote:
If 'last_processed_hostdev_vf != -1' is false then, since the loop counter 'i' starts at 0, 'i <= last_processed_hostdev_vf' can't possibly be true and the loop body will never be executed.
Hence, the first check is completely redundant and can be safely removed. --- src/util/virhostdev.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
Premise understood; however, Coverity has an issue...
Well, that's a first :P
Way back here:
507 virPCIDeviceListPtr pcidevs = NULL;
(1) Event var_tested_neg: Assigning: "last_processed_hostdev_vf" = a negative value. Also see events: [negative_returns]
508 int last_processed_hostdev_vf = -1;
Eventually we enter this loop:
for (i = 0; i < nhostdevs; i++) { virDomainHostdevDefPtr hostdev = hostdevs[i]; if (!virHostdevIsPCINetDevice(hostdev)) continue; if (virHostdevNetConfigReplace(hostdev, uuid, mgr->stateDir) < 0) { goto resetvfnetconfig; } last_processed_hostdev_vf = i; }
If for some reason we "continue" (or not) and eventually "goto resetvfnetconfig;" before ever setting last_processed_hostdev_vf, then we get to the goto.,..
resetvfnetconfig: - for (i = 0; - last_processed_hostdev_vf != -1 && i <= last_processed_hostdev_vf; i++) and last_processed_hostdev_vf still == -1
So that check needs to be there - perhaps just add an:
if (last_processed_hostdev_vf > -1) { }
I fail to see how this is a problem: if last_processed_hostdev_vf has never been assigned a value after being declared, then the rewritten loop would be equivalent to
for (i = 0; i <= -1; i++) { ... }
which means the loop body will never be executed, just as expected. Am I missing something?
size_t i; Coverity says: (60) Event negative_returns: Using unsigned variable "last_processed_hostdev_vf" in a loop exit condition. John
On Thu, 2016-03-10 at 12:35 -0500, John Ferlan wrote:
If for some reason we "continue" (or not) and eventually "goto resetvfnetconfig;" before ever setting last_processed_hostdev_vf, then we get to the goto.,..
resetvfnetconfig: - for (i = 0; - last_processed_hostdev_vf != -1 && i <= last_processed_hostdev_vf; i++) and last_processed_hostdev_vf still == -1 So that check needs to be there - perhaps just add an: if (last_processed_hostdev_vf > -1) { } I fail to see how this is a problem: if last_processed_hostdev_vf has never been assigned a value after being declared, then the rewritten loop would be equivalent to for (i = 0; i <= -1; i++) { ... } which means the loop body will never be executed, just as expected. Am I missing something? size_t i; Coverity says: (60) Event negative_returns: Using unsigned variable "last_processed_hostdev_vf" in a loop exit condition.
I admit I had to do some research, but I get it now. Not having any experience with the tool, its output looks very confusing to me[1]; thankfully you are fluent in Coverity and pointed me in the right direction :) Will change it to move the check on last_processed_hostdev_vf outside the for loop as you suggested, it might not be as nice as getting rid of it altogether but it's still much more readable than the current code. Cheers. [1] Why does it claim that 'last_processed_hostdev_vf' is an unsigned variable when it's not? And why would using an unsigned variable in loop exit condition automatically be a problem? -- Andrea Bolognani Software Engineer - Virtualization Team
On 03/10/2016 01:46 PM, Andrea Bolognani wrote:
On Thu, 2016-03-10 at 12:35 -0500, John Ferlan wrote:
If for some reason we "continue" (or not) and eventually "goto resetvfnetconfig;" before ever setting last_processed_hostdev_vf, then we get to the goto.,..
resetvfnetconfig: - for (i = 0; - last_processed_hostdev_vf != -1 && i <= last_processed_hostdev_vf; i++)
and last_processed_hostdev_vf still == -1
So that check needs to be there - perhaps just add an:
if (last_processed_hostdev_vf > -1) { } I fail to see how this is a problem: if last_processed_hostdev_vf has never been assigned a value after being declared, then the rewritten loop would be equivalent to
for (i = 0; i <= -1; i++) { ... }
which means the loop body will never be executed, just as expected. Am I missing something?
size_t i;
Coverity says:
(60) Event negative_returns: Using unsigned variable "last_processed_hostdev_vf" in a loop exit condition.
I admit I had to do some research, but I get it now.
Not having any experience with the tool, its output looks very confusing to me[1]; thankfully you are fluent in Coverity and pointed me in the right direction :)
Yeah it's like reading a whole new language sometimes. Especially confusing when you lookup the last*vf and see it's an 'int'. I would have felt better if it complained using an 'unsigned int' (i) and that shouldn't be compared with an 'int' (last*vf).
Will change it to move the check on last_processed_hostdev_vf outside the for loop as you suggested, it might not be as nice as getting rid of it altogether but it's still much more readable than the current code.
Well you could always go with a while loop and remove from the end.. while (last*vf >= 0) { hostdev = hostdevs[last*vf--]; virHostdevNetConfigRestore(hostdev,...); }
Cheers.
[1] Why does it claim that 'last_processed_hostdev_vf' is an unsigned variable when it's not? And why would using an unsigned variable in loop exit condition automatically be a problem?
Asking the wrong guy about compilers, size_t's, ssize_t's, int's, unsigned int's and what happens. In the long run it's all about compilers and types. That loop probably ends up being controlled in compiler code by using a 64-bit register doing unsigned variable comparisons (since that's what 'i' is). The last*vf will get moved into another register as an unsigned. That's a WAG. John
The only possible return values are true and false, so the return type should be bool instead of int. --- src/util/virhostdev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index f08502b..d3e15b7 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -354,7 +354,7 @@ virHostdevNetDevice(virDomainHostdevDefPtr hostdev, char **linkdev, } -static int +static bool virHostdevIsPCINetDevice(virDomainHostdevDefPtr hostdev) { return hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && -- 2.5.0
On 03/07/2016 12:24 PM, Andrea Bolognani wrote:
The only possible return values are true and false, so the return type should be bool instead of int. --- src/util/virhostdev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
ACK John
The new order aligns better with the virHostdev prefix. --- src/util/virhostdev.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index d3e15b7..e2accb4 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -750,7 +750,8 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, * are locked */ static void -virHostdevReattachPCIDevice(virPCIDevicePtr dev, virHostdevManagerPtr mgr) +virHostdevReattachPCIDevice(virHostdevManagerPtr mgr, + virPCIDevicePtr dev) { /* If the device is not managed and was attached to guest * successfully, it must have been inactive. @@ -890,7 +891,7 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr, */ while (virPCIDeviceListCount(pcidevs) > 0) { virPCIDevicePtr dev = virPCIDeviceListStealIndex(pcidevs, 0); - virHostdevReattachPCIDevice(dev, hostdev_mgr); + virHostdevReattachPCIDevice(hostdev_mgr, dev); } virObjectUnref(pcidevs); -- 2.5.0
On 03/07/2016 12:24 PM, Andrea Bolognani wrote:
The new order aligns better with the virHostdev prefix. --- src/util/virhostdev.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
ACK John
When we want to look up a device in a device list and we already have the IDs from another source, we can simply use virPCIDeviceListFindByIDs() instead of creating a temporary device object. --- src/util/virhostdev.c | 59 ++++++++++++++++++++++++--------------------------- 1 file changed, 28 insertions(+), 31 deletions(-) diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index e2accb4..fef7898 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -64,15 +64,11 @@ static int virHostdevIsPCINodeDeviceUsed(virPCIDeviceAddressPtr devAddr, void *o { virPCIDevicePtr other; int ret = -1; - virPCIDevicePtr pci = NULL; struct virHostdevIsPCINodeDeviceUsedData *helperData = opaque; - if (!(pci = virPCIDeviceNew(devAddr->domain, devAddr->bus, - devAddr->slot, devAddr->function))) - goto cleanup; - - other = virPCIDeviceListFind(helperData->hostdev_mgr->activePCIHostdevs, - pci); + other = virPCIDeviceListFindByIDs(helperData->hostdev_mgr->activePCIHostdevs, + devAddr->domain, devAddr->bus, + devAddr->slot, devAddr->function); if (other) { const char *other_drvname = NULL; const char *other_domname = NULL; @@ -87,18 +83,17 @@ static int virHostdevIsPCINodeDeviceUsed(virPCIDeviceAddressPtr devAddr, void *o virReportError(VIR_ERR_OPERATION_INVALID, _("PCI device %s is in use by " "driver %s, domain %s"), - virPCIDeviceGetName(pci), + virPCIDeviceGetName(other), other_drvname, other_domname); else virReportError(VIR_ERR_OPERATION_INVALID, _("PCI device %s is in use"), - virPCIDeviceGetName(pci)); + virPCIDeviceGetName(other)); goto cleanup; } iommu_owner: ret = 0; cleanup: - virPCIDeviceFree(pci); return ret; } @@ -669,7 +664,6 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, /* Step 8: Now set the original states for hostdev def */ for (i = 0; i < nhostdevs; i++) { virPCIDevicePtr dev; - virPCIDevicePtr pcidev; virDomainHostdevDefPtr hostdev = hostdevs[i]; virDomainHostdevSubsysPCIPtr pcisrc = &hostdev->source.subsys.u.pci; @@ -678,24 +672,25 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) continue; - dev = virPCIDeviceNew(pcisrc->addr.domain, pcisrc->addr.bus, - pcisrc->addr.slot, pcisrc->addr.function); + dev = virPCIDeviceListFindByIDs(pcidevs, + pcisrc->addr.domain, + pcisrc->addr.bus, + pcisrc->addr.slot, + pcisrc->addr.function); /* Appropriate values for the unbind_from_stub, remove_slot * and reprobe properties of the device were set earlier * by virPCIDeviceDetach() */ - VIR_DEBUG("Saving network configuration of PCI device %s", - virPCIDeviceGetName(dev)); - if ((pcidev = virPCIDeviceListFind(pcidevs, dev))) { + if (dev) { + VIR_DEBUG("Saving network configuration of PCI device %s", + virPCIDeviceGetName(dev)); hostdev->origstates.states.pci.unbind_from_stub = - virPCIDeviceGetUnbindFromStub(pcidev); + virPCIDeviceGetUnbindFromStub(dev); hostdev->origstates.states.pci.remove_slot = - virPCIDeviceGetRemoveSlot(pcidev); + virPCIDeviceGetRemoveSlot(dev); hostdev->origstates.states.pci.reprobe = - virPCIDeviceGetReprobe(pcidev); + virPCIDeviceGetReprobe(dev); } - - virPCIDeviceFree(dev); } /* Step 9: Now steal all the devices from pcidevs */ @@ -857,18 +852,20 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr, if (virHostdevIsPCINetDevice(hostdev)) { virDomainHostdevSubsysPCIPtr pcisrc = &hostdev->source.subsys.u.pci; - virPCIDevicePtr dev = NULL; - dev = virPCIDeviceNew(pcisrc->addr.domain, pcisrc->addr.bus, - pcisrc->addr.slot, pcisrc->addr.function); + virPCIDevicePtr dev; + + dev = virPCIDeviceListFindByIDs(pcidevs, + pcisrc->addr.domain, + pcisrc->addr.bus, + pcisrc->addr.slot, + pcisrc->addr.function); + if (dev) { - if (virPCIDeviceListFind(pcidevs, dev)) { - VIR_DEBUG("Restoring network configuration of PCI device %s", - virPCIDeviceGetName(dev)); - virHostdevNetConfigRestore(hostdev, hostdev_mgr->stateDir, - oldStateDir); - } + VIR_DEBUG("Restoring network configuration of PCI device %s", + virPCIDeviceGetName(dev)); + virHostdevNetConfigRestore(hostdev, hostdev_mgr->stateDir, + oldStateDir); } - virPCIDeviceFree(dev); } } -- 2.5.0
On 03/07/2016 12:24 PM, Andrea Bolognani wrote:
When we want to look up a device in a device list and we already have the IDs from another source, we can simply use virPCIDeviceListFindByIDs() instead of creating a temporary device object. --- src/util/virhostdev.c | 59 ++++++++++++++++++++++++--------------------------- 1 file changed, 28 insertions(+), 31 deletions(-)
ACK John
virHostdevGetPCIHostDeviceList() is similar but does not filter out devices that are not in the active list; that said, we are looking up the device in the active list just a few lines after anyway, so we might as well just keep a single function around. This also helps stress the fact the objects contained in pcidevs are only for looking up the actual devices, which is something later commits will make even more explicit. --- src/util/virhostdev.c | 50 ++++---------------------------------------------- 1 file changed, 4 insertions(+), 46 deletions(-) diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index fef7898..67e6e7b 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -245,49 +245,6 @@ virHostdevGetPCIHostDeviceList(virDomainHostdevDefPtr *hostdevs, int nhostdevs) } -/* - * virHostdevGetActivePCIHostDeviceList - make a new list with a *copy* of - * every virPCIDevice object that is found on the activePCIHostdevs - * list *and* is in the hostdev list for this domain. - * - * Return the new list, or NULL if there was a failure. - * - * Pre-condition: activePCIHostdevs is locked - */ -static virPCIDeviceListPtr -virHostdevGetActivePCIHostDeviceList(virHostdevManagerPtr mgr, - virDomainHostdevDefPtr *hostdevs, - int nhostdevs) -{ - virPCIDeviceListPtr list; - size_t i; - - if (!(list = virPCIDeviceListNew())) - return NULL; - - for (i = 0; i < nhostdevs; i++) { - virDomainHostdevDefPtr hostdev = hostdevs[i]; - virDevicePCIAddressPtr addr; - virPCIDevicePtr activeDev; - - if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) - continue; - if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) - continue; - - addr = &hostdev->source.subsys.u.pci.addr; - activeDev = virPCIDeviceListFindByIDs(mgr->activePCIHostdevs, - addr->domain, addr->bus, - addr->slot, addr->function); - if (activeDev && virPCIDeviceListAddCopy(list, activeDev) < 0) { - virObjectUnref(list); - return NULL; - } - } - - return list; -} - static int virHostdevPCISysfsPath(virDomainHostdevDefPtr hostdev, char **sysfs_path) @@ -800,9 +757,7 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr, virObjectLock(hostdev_mgr->activePCIHostdevs); virObjectLock(hostdev_mgr->inactivePCIHostdevs); - if (!(pcidevs = virHostdevGetActivePCIHostDeviceList(hostdev_mgr, - hostdevs, - nhostdevs))) { + if (!(pcidevs = virHostdevGetPCIHostDeviceList(hostdevs, nhostdevs))) { virErrorPtr err = virGetLastError(); VIR_ERROR(_("Failed to allocate PCI device list: %s"), err ? err->message : _("unknown error")); @@ -832,6 +787,9 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr, virPCIDeviceListDel(pcidevs, dev); continue; } + } else { + virPCIDeviceListDel(pcidevs, dev); + continue; } VIR_DEBUG("Removing PCI device %s from active list", -- 2.5.0
On 03/07/2016 12:24 PM, Andrea Bolognani wrote:
virHostdevGetPCIHostDeviceList() is similar but does not filter out devices that are not in the active list; that said, we are looking up the device in the active list just a few lines after anyway, so we might as well just keep a single function around.
This also helps stress the fact the objects contained in pcidevs are only for looking up the actual devices, which is something later commits will make even more explicit. --- src/util/virhostdev.c | 50 ++++---------------------------------------------- 1 file changed, 4 insertions(+), 46 deletions(-)
Existing code uses virPCIDeviceListAddCopy (as does code that populates inactiveDevs list). The AddCopy code will 1. virPCIDeviceCopy(virPCIDevicePtr dev) VIR_ALLOC(copy); *copy = *dev; Update copy->path, copy->used_by_drvname, & copy->used_by_domname 2. Add to a virPCIDeviceListPtr The new code. 1. virPCIDeviceNew for a virPCIDevicePtr pci; VIR_ALLOC(dev); copy in dev->address generate dev->name, dev->id (similar to copy) generate dev->path from dev->name 2. Add to a virPCIDeviceListPtr 3. Caller sets the managed and stubDriver backend. NOTE: there's no copy of 'used_by_drvname' and 'used_by_domname', like the copy function nor are a few other fields set. This seems to be important later [1].
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index fef7898..67e6e7b 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -245,49 +245,6 @@ virHostdevGetPCIHostDeviceList(virDomainHostdevDefPtr *hostdevs, int nhostdevs) }
-/* - * virHostdevGetActivePCIHostDeviceList - make a new list with a *copy* of - * every virPCIDevice object that is found on the activePCIHostdevs - * list *and* is in the hostdev list for this domain. - * - * Return the new list, or NULL if there was a failure. - * - * Pre-condition: activePCIHostdevs is locked - */ -static virPCIDeviceListPtr -virHostdevGetActivePCIHostDeviceList(virHostdevManagerPtr mgr, - virDomainHostdevDefPtr *hostdevs, - int nhostdevs) -{ - virPCIDeviceListPtr list; - size_t i; - - if (!(list = virPCIDeviceListNew())) - return NULL; - - for (i = 0; i < nhostdevs; i++) { - virDomainHostdevDefPtr hostdev = hostdevs[i]; - virDevicePCIAddressPtr addr; - virPCIDevicePtr activeDev; - - if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) - continue; - if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) - continue; - - addr = &hostdev->source.subsys.u.pci.addr; - activeDev = virPCIDeviceListFindByIDs(mgr->activePCIHostdevs, - addr->domain, addr->bus, - addr->slot, addr->function); - if (activeDev && virPCIDeviceListAddCopy(list, activeDev) < 0) { - virObjectUnref(list); - return NULL; - } - } - - return list; -} - static int virHostdevPCISysfsPath(virDomainHostdevDefPtr hostdev, char **sysfs_path) @@ -800,9 +757,7 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr, virObjectLock(hostdev_mgr->activePCIHostdevs); virObjectLock(hostdev_mgr->inactivePCIHostdevs);
- if (!(pcidevs = virHostdevGetActivePCIHostDeviceList(hostdev_mgr, - hostdevs, - nhostdevs))) { + if (!(pcidevs = virHostdevGetPCIHostDeviceList(hostdevs, nhostdevs))) { virErrorPtr err = virGetLastError(); VIR_ERROR(_("Failed to allocate PCI device list: %s"), err ? err->message : _("unknown error")); @@ -832,6 +787,9 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr,
[1] This code checks for usedby_drvname and usedby_domname These are set by virHostdevPreparePCIDevices and virHostdevUpdateActivePCIDevices. The virHostdevPreparePCIDevices is the only current caller of virHostdevGetPCIHostDeviceList. The latter will call the virPCIDeviceNew and then set the Managed, UsedBy, and stubDriver fields. The PreparePCIDevices code seems to do many of the same functions for the activePCIHostdevs. I think this one needs to be rethought.. John
virPCIDeviceListDel(pcidevs, dev); continue; } + } else { + virPCIDeviceListDel(pcidevs, dev); + continue; }
VIR_DEBUG("Removing PCI device %s from active list",
On 03/10/2016 01:54 PM, John Ferlan wrote:
On 03/07/2016 12:24 PM, Andrea Bolognani wrote:
virHostdevGetPCIHostDeviceList() is similar but does not filter out devices that are not in the active list; that said, we are looking up the device in the active list just a few lines after anyway, so we might as well just keep a single function around.
This also helps stress the fact the objects contained in pcidevs are only for looking up the actual devices, which is something later commits will make even more explicit. --- src/util/virhostdev.c | 50 ++++---------------------------------------------- 1 file changed, 4 insertions(+), 46 deletions(-)
<SIGH> Should have read patch 18 & 19 first... Looks like I'm getting confuzzled by all these lists and multitude of ways names were generated. The comparison being done is against the copy that came from the activePCIHostdev list which will have the fields I was concerned about. So in retrospect... ACK John
Existing code uses virPCIDeviceListAddCopy (as does code that populates inactiveDevs list). The AddCopy code will
1. virPCIDeviceCopy(virPCIDevicePtr dev) VIR_ALLOC(copy); *copy = *dev; Update copy->path, copy->used_by_drvname, & copy->used_by_domname
2. Add to a virPCIDeviceListPtr
The new code.
1. virPCIDeviceNew for a virPCIDevicePtr pci; VIR_ALLOC(dev); copy in dev->address generate dev->name, dev->id (similar to copy) generate dev->path from dev->name
2. Add to a virPCIDeviceListPtr
3. Caller sets the managed and stubDriver backend.
NOTE: there's no copy of 'used_by_drvname' and 'used_by_domname', like the copy function nor are a few other fields set. This seems to be important later [1].
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index fef7898..67e6e7b 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -245,49 +245,6 @@ virHostdevGetPCIHostDeviceList(virDomainHostdevDefPtr *hostdevs, int nhostdevs) }
-/* - * virHostdevGetActivePCIHostDeviceList - make a new list with a *copy* of - * every virPCIDevice object that is found on the activePCIHostdevs - * list *and* is in the hostdev list for this domain. - * - * Return the new list, or NULL if there was a failure. - * - * Pre-condition: activePCIHostdevs is locked - */ -static virPCIDeviceListPtr -virHostdevGetActivePCIHostDeviceList(virHostdevManagerPtr mgr, - virDomainHostdevDefPtr *hostdevs, - int nhostdevs) -{ - virPCIDeviceListPtr list; - size_t i; - - if (!(list = virPCIDeviceListNew())) - return NULL; - - for (i = 0; i < nhostdevs; i++) { - virDomainHostdevDefPtr hostdev = hostdevs[i]; - virDevicePCIAddressPtr addr; - virPCIDevicePtr activeDev; - - if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) - continue; - if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) - continue; - - addr = &hostdev->source.subsys.u.pci.addr; - activeDev = virPCIDeviceListFindByIDs(mgr->activePCIHostdevs, - addr->domain, addr->bus, - addr->slot, addr->function); - if (activeDev && virPCIDeviceListAddCopy(list, activeDev) < 0) { - virObjectUnref(list); - return NULL; - } - } - - return list; -} - static int virHostdevPCISysfsPath(virDomainHostdevDefPtr hostdev, char **sysfs_path) @@ -800,9 +757,7 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr, virObjectLock(hostdev_mgr->activePCIHostdevs); virObjectLock(hostdev_mgr->inactivePCIHostdevs);
- if (!(pcidevs = virHostdevGetActivePCIHostDeviceList(hostdev_mgr, - hostdevs, - nhostdevs))) { + if (!(pcidevs = virHostdevGetPCIHostDeviceList(hostdevs, nhostdevs))) { virErrorPtr err = virGetLastError(); VIR_ERROR(_("Failed to allocate PCI device list: %s"), err ? err->message : _("unknown error")); @@ -832,6 +787,9 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr,
[1] This code checks for usedby_drvname and usedby_domname
These are set by virHostdevPreparePCIDevices and virHostdevUpdateActivePCIDevices. The virHostdevPreparePCIDevices is the only current caller of virHostdevGetPCIHostDeviceList. The latter will call the virPCIDeviceNew and then set the Managed, UsedBy, and stubDriver fields.
The PreparePCIDevices code seems to do many of the same functions for the activePCIHostdevs.
I think this one needs to be rethought..
John
virPCIDeviceListDel(pcidevs, dev); continue; } + } else { + virPCIDeviceListDel(pcidevs, dev); + continue; }
VIR_DEBUG("Removing PCI device %s from active list",
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 03/10/2016 02:25 PM, John Ferlan wrote:
On 03/10/2016 01:54 PM, John Ferlan wrote:
On 03/07/2016 12:24 PM, Andrea Bolognani wrote:
virHostdevGetPCIHostDeviceList() is similar but does not filter out devices that are not in the active list; that said, we are looking up the device in the active list just a few lines after anyway, so we might as well just keep a single function around.
This also helps stress the fact the objects contained in pcidevs are only for looking up the actual devices, which is something later commits will make even more explicit. --- src/util/virhostdev.c | 50 ++++---------------------------------------------- 1 file changed, 4 insertions(+), 46 deletions(-)
Please read my logic in patch 20 before doing anything - I'm in the middle of it... scroll down for my short thoughts [1]... John
<SIGH> Should have read patch 18 & 19 first... Looks like I'm getting confuzzled by all these lists and multitude of ways names were generated. The comparison being done is against the copy that came from the activePCIHostdev list which will have the fields I was concerned about. So in retrospect...
ACK
John
Existing code uses virPCIDeviceListAddCopy (as does code that populates inactiveDevs list). The AddCopy code will
1. virPCIDeviceCopy(virPCIDevicePtr dev) VIR_ALLOC(copy); *copy = *dev; Update copy->path, copy->used_by_drvname, & copy->used_by_domname
2. Add to a virPCIDeviceListPtr
The new code.
1. virPCIDeviceNew for a virPCIDevicePtr pci; VIR_ALLOC(dev); copy in dev->address generate dev->name, dev->id (similar to copy) generate dev->path from dev->name
2. Add to a virPCIDeviceListPtr
3. Caller sets the managed and stubDriver backend.
NOTE: there's no copy of 'used_by_drvname' and 'used_by_domname', like the copy function nor are a few other fields set. This seems to be important later [1].
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index fef7898..67e6e7b 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -245,49 +245,6 @@ virHostdevGetPCIHostDeviceList(virDomainHostdevDefPtr *hostdevs, int nhostdevs) }
-/* - * virHostdevGetActivePCIHostDeviceList - make a new list with a *copy* of - * every virPCIDevice object that is found on the activePCIHostdevs - * list *and* is in the hostdev list for this domain. - * - * Return the new list, or NULL if there was a failure. - * - * Pre-condition: activePCIHostdevs is locked - */ -static virPCIDeviceListPtr -virHostdevGetActivePCIHostDeviceList(virHostdevManagerPtr mgr, - virDomainHostdevDefPtr *hostdevs, - int nhostdevs) -{ - virPCIDeviceListPtr list; - size_t i; - - if (!(list = virPCIDeviceListNew())) - return NULL; - - for (i = 0; i < nhostdevs; i++) { - virDomainHostdevDefPtr hostdev = hostdevs[i]; - virDevicePCIAddressPtr addr; - virPCIDevicePtr activeDev; - - if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) - continue; - if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) - continue; - - addr = &hostdev->source.subsys.u.pci.addr; - activeDev = virPCIDeviceListFindByIDs(mgr->activePCIHostdevs, - addr->domain, addr->bus, - addr->slot, addr->function); - if (activeDev && virPCIDeviceListAddCopy(list, activeDev) < 0) { - virObjectUnref(list); - return NULL; - } - } - - return list; -} - static int virHostdevPCISysfsPath(virDomainHostdevDefPtr hostdev, char **sysfs_path) @@ -800,9 +757,7 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr, virObjectLock(hostdev_mgr->activePCIHostdevs); virObjectLock(hostdev_mgr->inactivePCIHostdevs);
- if (!(pcidevs = virHostdevGetActivePCIHostDeviceList(hostdev_mgr, - hostdevs, - nhostdevs))) { + if (!(pcidevs = virHostdevGetPCIHostDeviceList(hostdevs, nhostdevs))) { virErrorPtr err = virGetLastError(); VIR_ERROR(_("Failed to allocate PCI device list: %s"), err ? err->message : _("unknown error")); @@ -832,6 +787,9 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr,
[1] This code checks for usedby_drvname and usedby_domname
These are set by virHostdevPreparePCIDevices and virHostdevUpdateActivePCIDevices. The virHostdevPreparePCIDevices is the only current caller of virHostdevGetPCIHostDeviceList. The latter will call the virPCIDeviceNew and then set the Managed, UsedBy, and stubDriver fields.
The PreparePCIDevices code seems to do many of the same functions for the activePCIHostdevs.
I think this one needs to be rethought..
John
virPCIDeviceListDel(pcidevs, dev); continue;
[1] this...
} + } else { + virPCIDeviceListDel(pcidevs, dev); + continue;
[1] ... and this mean...
}
VIR_DEBUG("Removing PCI device %s from active list",
[1] ... this doesn't happen!
On Fri, 2016-03-11 at 14:01 -0500, John Ferlan wrote:
Please read my logic in patch 20 before doing anything - I'm in the middle of it... scroll down for my short thoughts [1]...
Your comments to patch 20 are extremely detailed, so it will take me a while to go through them and reply properly. In the meantime, I'll give a short reply to your short thoughts :)
virPCIDeviceListDel(pcidevs, dev); continue; [1] this... } + } else { + virPCIDeviceListDel(pcidevs, dev); + continue; [1] ... and this mean... } VIR_DEBUG("Removing PCI device %s from active list", [1] ... this doesn't happen!
The code goes like this: activeDev = virPCIDeviceListFind(mgr->activePCIHostdevs, dev); if (activeDev) { const char *usedby_drvname; const char *usedby_domname; virPCIDeviceGetUsedBy(activeDev, &usedby_drvname, &usedby_domname); if (STRNEQ_NULLABLE(drv_name, usedby_drvname) || STRNEQ_NULLABLE(dom_name, usedby_domname)) { virPCIDeviceListDel(pcidevs, dev); continue; } } else { virPCIDeviceListDel(pcidevs, dev); continue; } VIR_DEBUG("Removing PCI device %s from active list", virPCIDeviceGetName(dev)); virPCIDeviceListDel(mgr->activePCIHostdevs, dev); i++; So 'dev' is used too look up 'activeDev' (should be 'actual', but the variable renaming patch has not been applyed at this point in time) in the active list. If no match is found, it means that the PCI device 'dev' is referring to is not actually active, and we should remove it from 'pcidevs' (so that it doesn't get processed further) and move on to the next one. If a match is found, but the actual device is used either by a different driver or by a different domain, we do the same thing: remove 'dev' from 'pcidevs' and move on. If neither of the above is true, namely if the device *is* active and *is* used by the driver and domain we're processing, *then* we proceed to remove it from the active list and, later, reattach it to the host. It should be noted that "making sure the devices we're about to reattach to the host are the ones this domain is using" and "remove devices from the active list" are split in patch 22, with the latter being joined with "add devices to the inactive list". All in all, I believe this is still correct and can be pushed along with patches 18 and 19 (after taking care of your comments there) before moving on with the more troublesome patch 20. Let me know whether you agree :) Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
On 03/14/2016 11:42 AM, Andrea Bolognani wrote:
On Fri, 2016-03-11 at 14:01 -0500, John Ferlan wrote:
Please read my logic in patch 20 before doing anything - I'm in the middle of it... scroll down for my short thoughts [1]...
Your comments to patch 20 are extremely detailed, so it will take me a while to go through them and reply properly.
In the meantime, I'll give a short reply to your short thoughts :)
virPCIDeviceListDel(pcidevs, dev); continue;
[1] this...
} + } else { + virPCIDeviceListDel(pcidevs, dev); + continue;
[1] ... and this mean...
}
VIR_DEBUG("Removing PCI device %s from active list",
[1] ... this doesn't happen!
The code goes like this:
activeDev = virPCIDeviceListFind(mgr->activePCIHostdevs, dev); if (activeDev) { const char *usedby_drvname; const char *usedby_domname; virPCIDeviceGetUsedBy(activeDev, &usedby_drvname, &usedby_domname); if (STRNEQ_NULLABLE(drv_name, usedby_drvname) || STRNEQ_NULLABLE(dom_name, usedby_domname)) {
virPCIDeviceListDel(pcidevs, dev); continue; } } else { virPCIDeviceListDel(pcidevs, dev); continue; }
VIR_DEBUG("Removing PCI device %s from active list", virPCIDeviceGetName(dev)); virPCIDeviceListDel(mgr->activePCIHostdevs, dev); i++;
So 'dev' is used too look up 'activeDev' (should be 'actual', but the variable renaming patch has not been applyed at this point in time) in the active list.
If no match is found, it means that the PCI device 'dev' is referring to is not actually active, and we should remove it from 'pcidevs' (so that it doesn't get processed further) and move on to the next one.
If a match is found, but the actual device is used either by a different driver or by a different domain, we do the same thing: remove 'dev' from 'pcidevs' and move on.
If neither of the above is true, namely if the device *is* active and *is* used by the driver and domain we're processing, *then* we proceed to remove it from the active list and, later, reattach it to the host.
It should be noted that "making sure the devices we're about to reattach to the host are the ones this domain is using" and "remove devices from the active list" are split in patch 22, with the latter being joined with "add devices to the inactive list".
All in all, I believe this is still correct and can be pushed along with patches 18 and 19 (after taking care of your comments there) before moving on with the more troublesome patch 20. Let me know whether you agree :)
I probably changed my mind 20 times while reviewing 20-22. I think in retrospect my secondary comments were incorrect since we could get to that removal of the device from the active list if the drv_name and dom_name match. I think later when we move from activeList to inactiveList things are a bit clearer, but like we've already agreed upon - this is code that once you step in it, it's hard to get it off the bottom of your foot. So, I agree let's ACK this one and move on. It's worth noting that once this patch is complete 'pcidevs' will be the list of activeDevs that were removed. John
On Mon, 2016-03-14 at 19:40 -0400, John Ferlan wrote:
All in all, I believe this is still correct and can be pushed along with patches 18 and 19 (after taking care of your comments there) before moving on with the more troublesome patch 20. Let me know whether you agree :) I probably changed my mind 20 times while reviewing 20-22. I think in retrospect my secondary comments were incorrect since we could get to that removal of the device from the active list if the drv_name and dom_name match.
Exactly.
I think later when we move from activeList to inactiveList things are a bit clearer, but like we've already agreed upon - this is code that once you step in it, it's hard to get it off the bottom of your foot. So, I agree let's ACK this one and move on. It's worth noting that once this patch is complete 'pcidevs' will be the list of activeDevs that were removed.
I've pushed everything up to patch 19 now - still need to go through your comments on patches 21 and 22, and of course address the problem you've spotted in patch 20. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
On Thu, 2016-03-10 at 14:25 -0500, John Ferlan wrote:
<SIGH> Should have read patch 18 & 19 first... Looks like I'm getting confuzzled by all these lists and multitude of ways names were generated. The comparison being done is against the copy that came from the activePCIHostdev list which will have the fields I was concerned about. So in retrospect... ACK
To be fair, patches 18 and 19 were written *exactly* to make it a tiny bit easier to follow along, especially with patch 21 in mind. It would probably have been better if the name changes had been made *before* changing the code too much, as this patch is doing, but the comments in patch 19 would not really be true if patch 15 had not been applied first, so... Rock and hard place, really :( I'll reply to your other concern down the thread, consider this just an aside. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
We're in the hostdev module, so mgr is not an ambiguous name, and in fact it's already used in some cases. Switch all the code over. Take the chance to shorten declaration of virHostdevIsPCINodeDeviceUsedData structures. --- src/util/virhostdev.c | 155 +++++++++++++++++++++++++------------------------- 1 file changed, 76 insertions(+), 79 deletions(-) diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index 67e6e7b..46a385c 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -55,7 +55,7 @@ static void virHostdevManagerDispose(void *obj); static virHostdevManagerPtr virHostdevManagerNew(void); struct virHostdevIsPCINodeDeviceUsedData { - virHostdevManagerPtr hostdev_mgr; + virHostdevManagerPtr mgr; const char *domainName; const bool usesVfio; }; @@ -66,7 +66,7 @@ static int virHostdevIsPCINodeDeviceUsed(virPCIDeviceAddressPtr devAddr, void *o int ret = -1; struct virHostdevIsPCINodeDeviceUsedData *helperData = opaque; - other = virPCIDeviceListFindByIDs(helperData->hostdev_mgr->activePCIHostdevs, + other = virPCIDeviceListFindByIDs(helperData->mgr->activePCIHostdevs, devAddr->domain, devAddr->bus, devAddr->slot, devAddr->function); if (other) { @@ -426,7 +426,7 @@ virHostdevNetConfigReplace(virDomainHostdevDefPtr hostdev, * For upgrade purpose: * To an existing VM on QEMU, the hostdev netconfig file is originally stored * in cfg->stateDir (/var/run/libvirt/qemu). Switch to new version, it uses new - * location (hostdev_mgr->stateDir) but certainly will not find it. In this + * location (mgr->stateDir) but certainly will not find it. In this * case, try to find in the old state dir. */ static int @@ -475,7 +475,7 @@ virHostdevNetConfigRestore(virDomainHostdevDefPtr hostdev, } int -virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, +virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, const char *drv_name, const char *dom_name, const unsigned char *uuid, @@ -492,8 +492,8 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, if (!nhostdevs) return 0; - virObjectLock(hostdev_mgr->activePCIHostdevs); - virObjectLock(hostdev_mgr->inactivePCIHostdevs); + virObjectLock(mgr->activePCIHostdevs); + virObjectLock(mgr->inactivePCIHostdevs); if (!(pcidevs = virHostdevGetPCIHostDeviceList(hostdevs, nhostdevs))) goto cleanup; @@ -514,8 +514,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); bool strict_acs_check = !!(flags & VIR_HOSTDEV_STRICT_ACS_CHECK); bool usesVfio = (virPCIDeviceGetStubDriver(dev) == VIR_PCI_STUB_DRIVER_VFIO); - struct virHostdevIsPCINodeDeviceUsedData data = {hostdev_mgr, dom_name, - usesVfio}; + struct virHostdevIsPCINodeDeviceUsedData data = { mgr, dom_name, usesVfio }; if (!usesVfio && !virPCIDeviceIsAssignable(dev, strict_acs_check)) { virReportError(VIR_ERR_OPERATION_INVALID, @@ -548,8 +547,8 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, VIR_DEBUG("Detaching managed PCI device %s", virPCIDeviceGetName(dev)); if (virPCIDeviceDetach(dev, - hostdev_mgr->activePCIHostdevs, - hostdev_mgr->inactivePCIHostdevs) < 0) + mgr->activePCIHostdevs, + mgr->inactivePCIHostdevs) < 0) goto reattachdevs; } else { VIR_DEBUG("Not detaching unmanaged PCI device %s", @@ -566,8 +565,8 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); VIR_DEBUG("Resetting PCI device %s", virPCIDeviceGetName(dev)); - if (virPCIDeviceReset(dev, hostdev_mgr->activePCIHostdevs, - hostdev_mgr->inactivePCIHostdevs) < 0) + if (virPCIDeviceReset(dev, mgr->activePCIHostdevs, + mgr->inactivePCIHostdevs) < 0) goto reattachdevs; } @@ -578,7 +577,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, if (!virHostdevIsPCINetDevice(hostdev)) continue; if (virHostdevNetConfigReplace(hostdev, uuid, - hostdev_mgr->stateDir) < 0) { + mgr->stateDir) < 0) { goto resetvfnetconfig; } last_processed_hostdev_vf = i; @@ -590,7 +589,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, VIR_DEBUG("Adding PCI device %s to active list", virPCIDeviceGetName(dev)); - if (virPCIDeviceListAdd(hostdev_mgr->activePCIHostdevs, dev) < 0) + if (virPCIDeviceListAdd(mgr->activePCIHostdevs, dev) < 0) goto inactivedevs; } @@ -600,7 +599,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, VIR_DEBUG("Removing PCI device %s from inactive list", virPCIDeviceGetName(dev)); - virPCIDeviceListDel(hostdev_mgr->inactivePCIHostdevs, dev); + virPCIDeviceListDel(mgr->inactivePCIHostdevs, dev); } /* Step 7: Now set the used_by_domain of the device in @@ -610,7 +609,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, virPCIDevicePtr dev, activeDev; dev = virPCIDeviceListGet(pcidevs, i); - activeDev = virPCIDeviceListFind(hostdev_mgr->activePCIHostdevs, dev); + activeDev = virPCIDeviceListFind(mgr->activePCIHostdevs, dev); VIR_DEBUG("Setting driver and domain information for PCI device %s", virPCIDeviceGetName(dev)); @@ -666,12 +665,12 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, VIR_DEBUG("Removing PCI device %s from active list", virPCIDeviceGetName(dev)); - virPCIDeviceListSteal(hostdev_mgr->activePCIHostdevs, dev); + virPCIDeviceListSteal(mgr->activePCIHostdevs, dev); } resetvfnetconfig: for (i = 0; i <= last_processed_hostdev_vf; i++) - virHostdevNetConfigRestore(hostdevs[i], hostdev_mgr->stateDir, NULL); + virHostdevNetConfigRestore(hostdevs[i], mgr->stateDir, NULL); reattachdevs: for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { @@ -681,8 +680,8 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, VIR_DEBUG("Reattaching managed PCI device %s", virPCIDeviceGetName(dev)); ignore_value(virPCIDeviceReattach(dev, - hostdev_mgr->activePCIHostdevs, - hostdev_mgr->inactivePCIHostdevs)); + mgr->activePCIHostdevs, + mgr->inactivePCIHostdevs)); } else { VIR_DEBUG("Not reattaching unmanaged PCI device %s", virPCIDeviceGetName(dev)); @@ -691,8 +690,8 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, cleanup: virObjectUnref(pcidevs); - virObjectUnlock(hostdev_mgr->activePCIHostdevs); - virObjectUnlock(hostdev_mgr->inactivePCIHostdevs); + virObjectUnlock(mgr->activePCIHostdevs); + virObjectUnlock(mgr->inactivePCIHostdevs); return ret; } @@ -741,7 +740,7 @@ virHostdevReattachPCIDevice(virHostdevManagerPtr mgr, * For upgrade purpose: see virHostdevNetConfigRestore */ void -virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr, +virHostdevReAttachPCIDevices(virHostdevManagerPtr mgr, const char *drv_name, const char *dom_name, virDomainHostdevDefPtr *hostdevs, @@ -754,8 +753,8 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr, if (!nhostdevs) return; - virObjectLock(hostdev_mgr->activePCIHostdevs); - virObjectLock(hostdev_mgr->inactivePCIHostdevs); + virObjectLock(mgr->activePCIHostdevs); + virObjectLock(mgr->inactivePCIHostdevs); if (!(pcidevs = virHostdevGetPCIHostDeviceList(hostdevs, nhostdevs))) { virErrorPtr err = virGetLastError(); @@ -776,7 +775,7 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr, virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); virPCIDevicePtr activeDev = NULL; - activeDev = virPCIDeviceListFind(hostdev_mgr->activePCIHostdevs, dev); + activeDev = virPCIDeviceListFind(mgr->activePCIHostdevs, dev); if (activeDev) { const char *usedby_drvname; const char *usedby_domname; @@ -794,7 +793,7 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr, VIR_DEBUG("Removing PCI device %s from active list", virPCIDeviceGetName(dev)); - virPCIDeviceListDel(hostdev_mgr->activePCIHostdevs, dev); + virPCIDeviceListDel(mgr->activePCIHostdevs, dev); i++; } @@ -821,7 +820,7 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr, if (dev) { VIR_DEBUG("Restoring network configuration of PCI device %s", virPCIDeviceGetName(dev)); - virHostdevNetConfigRestore(hostdev, hostdev_mgr->stateDir, + virHostdevNetConfigRestore(hostdev, mgr->stateDir, oldStateDir); } } @@ -832,8 +831,8 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr, virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); VIR_DEBUG("Resetting PCI device %s", virPCIDeviceGetName(dev)); - if (virPCIDeviceReset(dev, hostdev_mgr->activePCIHostdevs, - hostdev_mgr->inactivePCIHostdevs) < 0) { + if (virPCIDeviceReset(dev, mgr->activePCIHostdevs, + mgr->inactivePCIHostdevs) < 0) { virErrorPtr err = virGetLastError(); VIR_ERROR(_("Failed to reset PCI device: %s"), err ? err->message : _("unknown error")); @@ -846,13 +845,13 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr, */ while (virPCIDeviceListCount(pcidevs) > 0) { virPCIDevicePtr dev = virPCIDeviceListStealIndex(pcidevs, 0); - virHostdevReattachPCIDevice(hostdev_mgr, dev); + virHostdevReattachPCIDevice(mgr, dev); } virObjectUnref(pcidevs); cleanup: - virObjectUnlock(hostdev_mgr->activePCIHostdevs); - virObjectUnlock(hostdev_mgr->inactivePCIHostdevs); + virObjectUnlock(mgr->activePCIHostdevs); + virObjectUnlock(mgr->inactivePCIHostdevs); } int @@ -1185,7 +1184,7 @@ virHostdevFindUSBDevice(virDomainHostdevDefPtr hostdev, } int -virHostdevPrepareUSBDevices(virHostdevManagerPtr hostdev_mgr, +virHostdevPrepareUSBDevices(virHostdevManagerPtr mgr, const char *drv_name, const char *dom_name, virDomainHostdevDefPtr *hostdevs, @@ -1239,7 +1238,7 @@ virHostdevPrepareUSBDevices(virHostdevManagerPtr hostdev_mgr, * and add them do driver list. However, if something goes * wrong, perform rollback. */ - if (virHostdevMarkUSBDevices(hostdev_mgr, drv_name, dom_name, list) < 0) + if (virHostdevMarkUSBDevices(mgr, drv_name, dom_name, list) < 0) goto cleanup; /* Loop 2: Temporary list was successfully merged with @@ -1291,7 +1290,7 @@ virHostdevPrepareSCSIHostDevices(virDomainHostdevDefPtr hostdev, } int -virHostdevPrepareSCSIDevices(virHostdevManagerPtr hostdev_mgr, +virHostdevPrepareSCSIDevices(virHostdevManagerPtr mgr, const char *drv_name, const char *dom_name, virDomainHostdevDefPtr *hostdevs, @@ -1334,12 +1333,12 @@ virHostdevPrepareSCSIDevices(virHostdevManagerPtr hostdev_mgr, * and add them to driver list. However, if something goes * wrong, perform rollback. */ - virObjectLock(hostdev_mgr->activeSCSIHostdevs); + virObjectLock(mgr->activeSCSIHostdevs); count = virSCSIDeviceListCount(list); for (i = 0; i < count; i++) { virSCSIDevicePtr scsi = virSCSIDeviceListGet(list, i); - if ((tmp = virSCSIDeviceListFind(hostdev_mgr->activeSCSIHostdevs, + if ((tmp = virSCSIDeviceListFind(mgr->activeSCSIHostdevs, scsi))) { bool scsi_shareable = virSCSIDeviceGetShareable(scsi); bool tmp_shareable = virSCSIDeviceGetShareable(tmp); @@ -1361,12 +1360,12 @@ virHostdevPrepareSCSIDevices(virHostdevManagerPtr hostdev_mgr, VIR_DEBUG("Adding %s to activeSCSIHostdevs", virSCSIDeviceGetName(scsi)); - if (virSCSIDeviceListAdd(hostdev_mgr->activeSCSIHostdevs, scsi) < 0) + if (virSCSIDeviceListAdd(mgr->activeSCSIHostdevs, scsi) < 0) goto error; } } - virObjectUnlock(hostdev_mgr->activeSCSIHostdevs); + virObjectUnlock(mgr->activeSCSIHostdevs); /* Loop 3: Temporary list was successfully merged with * driver list, so steal all items to avoid freeing them @@ -1383,16 +1382,16 @@ virHostdevPrepareSCSIDevices(virHostdevManagerPtr hostdev_mgr, error: for (j = 0; j < i; j++) { tmp = virSCSIDeviceListGet(list, i); - virSCSIDeviceListSteal(hostdev_mgr->activeSCSIHostdevs, tmp); + virSCSIDeviceListSteal(mgr->activeSCSIHostdevs, tmp); } - virObjectUnlock(hostdev_mgr->activeSCSIHostdevs); + virObjectUnlock(mgr->activeSCSIHostdevs); cleanup: virObjectUnref(list); return -1; } void -virHostdevReAttachUSBDevices(virHostdevManagerPtr hostdev_mgr, +virHostdevReAttachUSBDevices(virHostdevManagerPtr mgr, const char *drv_name, const char *dom_name, virDomainHostdevDefPtr *hostdevs, @@ -1403,7 +1402,7 @@ virHostdevReAttachUSBDevices(virHostdevManagerPtr hostdev_mgr, if (!nhostdevs) return; - virObjectLock(hostdev_mgr->activeUSBHostdevs); + virObjectLock(mgr->activeUSBHostdevs); for (i = 0; i < nhostdevs; i++) { virDomainHostdevDefPtr hostdev = hostdevs[i]; virDomainHostdevSubsysUSBPtr usbsrc = &hostdev->source.subsys.u.usb; @@ -1430,7 +1429,7 @@ virHostdevReAttachUSBDevices(virHostdevManagerPtr hostdev_mgr, * Therefore we want to steal only those devices from * the list which were taken by @name */ - tmp = virUSBDeviceListFind(hostdev_mgr->activeUSBHostdevs, usb); + tmp = virUSBDeviceListFind(mgr->activeUSBHostdevs, usb); virUSBDeviceFree(usb); if (!tmp) { @@ -1445,14 +1444,14 @@ virHostdevReAttachUSBDevices(virHostdevManagerPtr hostdev_mgr, STREQ_NULLABLE(dom_name, usedby_domname)) { VIR_DEBUG("Removing %03d.%03d dom=%s from activeUSBHostdevs", usbsrc->bus, usbsrc->device, dom_name); - virUSBDeviceListDel(hostdev_mgr->activeUSBHostdevs, tmp); + virUSBDeviceListDel(mgr->activeUSBHostdevs, tmp); } } - virObjectUnlock(hostdev_mgr->activeUSBHostdevs); + virObjectUnlock(mgr->activeUSBHostdevs); } static void -virHostdevReAttachSCSIHostDevices(virHostdevManagerPtr hostdev_mgr, +virHostdevReAttachSCSIHostDevices(virHostdevManagerPtr mgr, virDomainHostdevDefPtr hostdev, virDomainHostdevSubsysSCSIPtr scsisrc, const char *drv_name, @@ -1475,7 +1474,7 @@ virHostdevReAttachSCSIHostDevices(virHostdevManagerPtr hostdev_mgr, /* Only delete the devices which are marked as being used by @name, * because qemuProcessStart could fail half way through. */ - if (!(tmp = virSCSIDeviceListFind(hostdev_mgr->activeSCSIHostdevs, scsi))) { + if (!(tmp = virSCSIDeviceListFind(mgr->activeSCSIHostdevs, scsi))) { VIR_WARN("Unable to find device %s:%u:%u:%llu " "in list of active SCSI devices", scsihostsrc->adapter, scsihostsrc->bus, @@ -1488,13 +1487,13 @@ virHostdevReAttachSCSIHostDevices(virHostdevManagerPtr hostdev_mgr, scsihostsrc->adapter, scsihostsrc->bus, scsihostsrc->target, scsihostsrc->unit, dom_name); - virSCSIDeviceListDel(hostdev_mgr->activeSCSIHostdevs, tmp, + virSCSIDeviceListDel(mgr->activeSCSIHostdevs, tmp, drv_name, dom_name); virSCSIDeviceFree(scsi); } void -virHostdevReAttachSCSIDevices(virHostdevManagerPtr hostdev_mgr, +virHostdevReAttachSCSIDevices(virHostdevManagerPtr mgr, const char *drv_name, const char *dom_name, virDomainHostdevDefPtr *hostdevs, @@ -1505,7 +1504,7 @@ virHostdevReAttachSCSIDevices(virHostdevManagerPtr hostdev_mgr, if (!nhostdevs) return; - virObjectLock(hostdev_mgr->activeSCSIHostdevs); + virObjectLock(mgr->activeSCSIHostdevs); for (i = 0; i < nhostdevs; i++) { virDomainHostdevDefPtr hostdev = hostdevs[i]; virDomainHostdevSubsysSCSIPtr scsisrc = &hostdev->source.subsys.u.scsi; @@ -1517,49 +1516,47 @@ virHostdevReAttachSCSIDevices(virHostdevManagerPtr hostdev_mgr, if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI) continue; /* Not supported for iSCSI */ else - virHostdevReAttachSCSIHostDevices(hostdev_mgr, hostdev, scsisrc, + virHostdevReAttachSCSIHostDevices(mgr, hostdev, scsisrc, drv_name, dom_name); } - virObjectUnlock(hostdev_mgr->activeSCSIHostdevs); + virObjectUnlock(mgr->activeSCSIHostdevs); } int -virHostdevPCINodeDeviceDetach(virHostdevManagerPtr hostdev_mgr, +virHostdevPCINodeDeviceDetach(virHostdevManagerPtr mgr, virPCIDevicePtr pci) { - struct virHostdevIsPCINodeDeviceUsedData data = { hostdev_mgr, NULL, - false }; + struct virHostdevIsPCINodeDeviceUsedData data = { mgr, NULL, false }; int ret = -1; - virObjectLock(hostdev_mgr->activePCIHostdevs); - virObjectLock(hostdev_mgr->inactivePCIHostdevs); + virObjectLock(mgr->activePCIHostdevs); + virObjectLock(mgr->inactivePCIHostdevs); if (virHostdevIsPCINodeDeviceUsed(virPCIDeviceGetAddress(pci), &data)) goto cleanup; - if (virPCIDeviceDetach(pci, hostdev_mgr->activePCIHostdevs, - hostdev_mgr->inactivePCIHostdevs) < 0) + if (virPCIDeviceDetach(pci, mgr->activePCIHostdevs, + mgr->inactivePCIHostdevs) < 0) goto cleanup; ret = 0; cleanup: - virObjectUnlock(hostdev_mgr->inactivePCIHostdevs); - virObjectUnlock(hostdev_mgr->activePCIHostdevs); + virObjectUnlock(mgr->inactivePCIHostdevs); + virObjectUnlock(mgr->activePCIHostdevs); return ret; } int -virHostdevPCINodeDeviceReAttach(virHostdevManagerPtr hostdev_mgr, +virHostdevPCINodeDeviceReAttach(virHostdevManagerPtr mgr, virPCIDevicePtr pci) { - struct virHostdevIsPCINodeDeviceUsedData data = {hostdev_mgr, NULL, - false}; + struct virHostdevIsPCINodeDeviceUsedData data = { mgr, NULL, false }; int ret = -1; - virObjectLock(hostdev_mgr->activePCIHostdevs); - virObjectLock(hostdev_mgr->inactivePCIHostdevs); + virObjectLock(mgr->activePCIHostdevs); + virObjectLock(mgr->inactivePCIHostdevs); if (virHostdevIsPCINodeDeviceUsed(virPCIDeviceGetAddress(pci), &data)) goto cleanup; @@ -1568,35 +1565,35 @@ virHostdevPCINodeDeviceReAttach(virHostdevManagerPtr hostdev_mgr, virPCIDeviceSetRemoveSlot(pci, true); virPCIDeviceSetReprobe(pci, true); - if (virPCIDeviceReattach(pci, hostdev_mgr->activePCIHostdevs, - hostdev_mgr->inactivePCIHostdevs) < 0) + if (virPCIDeviceReattach(pci, mgr->activePCIHostdevs, + mgr->inactivePCIHostdevs) < 0) goto cleanup; ret = 0; cleanup: - virObjectUnlock(hostdev_mgr->inactivePCIHostdevs); - virObjectUnlock(hostdev_mgr->activePCIHostdevs); + virObjectUnlock(mgr->inactivePCIHostdevs); + virObjectUnlock(mgr->activePCIHostdevs); return ret; } int -virHostdevPCINodeDeviceReset(virHostdevManagerPtr hostdev_mgr, +virHostdevPCINodeDeviceReset(virHostdevManagerPtr mgr, virPCIDevicePtr pci) { int ret = -1; - virObjectLock(hostdev_mgr->activePCIHostdevs); - virObjectLock(hostdev_mgr->inactivePCIHostdevs); - if (virPCIDeviceReset(pci, hostdev_mgr->activePCIHostdevs, - hostdev_mgr->inactivePCIHostdevs) < 0) + virObjectLock(mgr->activePCIHostdevs); + virObjectLock(mgr->inactivePCIHostdevs); + if (virPCIDeviceReset(pci, mgr->activePCIHostdevs, + mgr->inactivePCIHostdevs) < 0) goto out; ret = 0; out: - virObjectUnlock(hostdev_mgr->inactivePCIHostdevs); - virObjectUnlock(hostdev_mgr->activePCIHostdevs); + virObjectUnlock(mgr->inactivePCIHostdevs); + virObjectUnlock(mgr->activePCIHostdevs); return ret; } -- 2.5.0
On 03/07/2016 12:24 PM, Andrea Bolognani wrote:
We're in the hostdev module, so mgr is not an ambiguous name, and in fact it's already used in some cases. Switch all the code over.
Take the chance to shorten declaration of virHostdevIsPCINodeDeviceUsedData structures. --- src/util/virhostdev.c | 155 +++++++++++++++++++++++++------------------------- 1 file changed, 76 insertions(+), 79 deletions(-)
ACK - although depending on what happens with patch 15 you probably want to hold off on pushing... John
On Thu, 2016-03-10 at 14:11 -0500, John Ferlan wrote:
On 03/07/2016 12:24 PM, Andrea Bolognani wrote:
We're in the hostdev module, so mgr is not an ambiguous name, and in fact it's already used in some cases. Switch all the code over.
Take the chance to shorten declaration of virHostdevIsPCINodeDeviceUsedData structures. --- src/util/virhostdev.c | 155 +++++++++++++++++++++++++------------------------- 1 file changed, 76 insertions(+), 79 deletions(-)
ACK - although depending on what happens with patch 15 you probably want to hold off on pushing...
I've just shuffled commits around, pushing everything up to patch 17 but excluding patch 15, to try and get as much trivial stuff as possible out of the way now that we're getting to the interesting bits :) I'm going to reply to your comments on patch 15 and following soon. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
Acronyms should be written in all caps. --- src/util/virhostdev.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index 46a385c..a7fb8b1 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -57,7 +57,7 @@ static virHostdevManagerPtr virHostdevManagerNew(void); struct virHostdevIsPCINodeDeviceUsedData { virHostdevManagerPtr mgr; const char *domainName; - const bool usesVfio; + const bool usesVFIO; }; static int virHostdevIsPCINodeDeviceUsed(virPCIDeviceAddressPtr devAddr, void *opaque) @@ -74,7 +74,7 @@ static int virHostdevIsPCINodeDeviceUsed(virPCIDeviceAddressPtr devAddr, void *o const char *other_domname = NULL; virPCIDeviceGetUsedBy(other, &other_drvname, &other_domname); - if (helperData->usesVfio && + if (helperData->usesVFIO && (other_domname && helperData->domainName) && (STREQ(other_domname, helperData->domainName))) goto iommu_owner; @@ -513,10 +513,10 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); bool strict_acs_check = !!(flags & VIR_HOSTDEV_STRICT_ACS_CHECK); - bool usesVfio = (virPCIDeviceGetStubDriver(dev) == VIR_PCI_STUB_DRIVER_VFIO); - struct virHostdevIsPCINodeDeviceUsedData data = { mgr, dom_name, usesVfio }; + bool usesVFIO = (virPCIDeviceGetStubDriver(dev) == VIR_PCI_STUB_DRIVER_VFIO); + struct virHostdevIsPCINodeDeviceUsedData data = { mgr, dom_name, usesVFIO }; - if (!usesVfio && !virPCIDeviceIsAssignable(dev, strict_acs_check)) { + if (!usesVFIO && !virPCIDeviceIsAssignable(dev, strict_acs_check)) { virReportError(VIR_ERR_OPERATION_INVALID, _("PCI device %s is not assignable"), virPCIDeviceGetName(dev)); @@ -529,7 +529,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, * across guests. */ devAddr = virPCIDeviceGetAddress(dev); - if (usesVfio) { + if (usesVFIO) { if (virPCIDeviceAddressIOMMUGroupIterate(devAddr, virHostdevIsPCINodeDeviceUsed, &data) < 0) -- 2.5.0
This is not just a cosmetic change: the name of the variable now gives a hint about what it is supposed to be used for. --- src/util/virhostdev.c | 234 +++++++++++++++++++++++++------------------------- 1 file changed, 117 insertions(+), 117 deletions(-) diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index a7fb8b1..10d1c1a 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -62,33 +62,33 @@ struct virHostdevIsPCINodeDeviceUsedData { static int virHostdevIsPCINodeDeviceUsed(virPCIDeviceAddressPtr devAddr, void *opaque) { - virPCIDevicePtr other; + virPCIDevicePtr actual; int ret = -1; struct virHostdevIsPCINodeDeviceUsedData *helperData = opaque; - other = virPCIDeviceListFindByIDs(helperData->mgr->activePCIHostdevs, - devAddr->domain, devAddr->bus, - devAddr->slot, devAddr->function); - if (other) { - const char *other_drvname = NULL; - const char *other_domname = NULL; - virPCIDeviceGetUsedBy(other, &other_drvname, &other_domname); + actual = virPCIDeviceListFindByIDs(helperData->mgr->activePCIHostdevs, + devAddr->domain, devAddr->bus, + devAddr->slot, devAddr->function); + if (actual) { + const char *actual_drvname = NULL; + const char *actual_domname = NULL; + virPCIDeviceGetUsedBy(actual, &actual_drvname, &actual_domname); if (helperData->usesVFIO && - (other_domname && helperData->domainName) && - (STREQ(other_domname, helperData->domainName))) + (actual_domname && helperData->domainName) && + (STREQ(actual_domname, helperData->domainName))) goto iommu_owner; - if (other_drvname && other_domname) + if (actual_drvname && actual_domname) virReportError(VIR_ERR_OPERATION_INVALID, _("PCI device %s is in use by " "driver %s, domain %s"), - virPCIDeviceGetName(other), - other_drvname, other_domname); + virPCIDeviceGetName(actual), + actual_drvname, actual_domname); else virReportError(VIR_ERR_OPERATION_INVALID, _("PCI device %s is in use"), - virPCIDeviceGetName(other)); + virPCIDeviceGetName(actual)); goto cleanup; } iommu_owner: @@ -203,45 +203,45 @@ virHostdevManagerGetDefault(void) static virPCIDeviceListPtr virHostdevGetPCIHostDeviceList(virDomainHostdevDefPtr *hostdevs, int nhostdevs) { - virPCIDeviceListPtr list; + virPCIDeviceListPtr pcidevs; size_t i; - if (!(list = virPCIDeviceListNew())) + if (!(pcidevs = virPCIDeviceListNew())) return NULL; for (i = 0; i < nhostdevs; i++) { virDomainHostdevDefPtr hostdev = hostdevs[i]; virDomainHostdevSubsysPCIPtr pcisrc = &hostdev->source.subsys.u.pci; - virPCIDevicePtr dev; + virPCIDevicePtr pci; if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) continue; if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) continue; - dev = virPCIDeviceNew(pcisrc->addr.domain, pcisrc->addr.bus, + pci = virPCIDeviceNew(pcisrc->addr.domain, pcisrc->addr.bus, pcisrc->addr.slot, pcisrc->addr.function); - if (!dev) { - virObjectUnref(list); + if (!pci) { + virObjectUnref(pcidevs); return NULL; } - if (virPCIDeviceListAdd(list, dev) < 0) { - virPCIDeviceFree(dev); - virObjectUnref(list); + if (virPCIDeviceListAdd(pcidevs, pci) < 0) { + virPCIDeviceFree(pci); + virObjectUnref(pcidevs); return NULL; } - virPCIDeviceSetManaged(dev, hostdev->managed); + virPCIDeviceSetManaged(pci, hostdev->managed); if (pcisrc->backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) - virPCIDeviceSetStubDriver(dev, VIR_PCI_STUB_DRIVER_VFIO); + virPCIDeviceSetStubDriver(pci, VIR_PCI_STUB_DRIVER_VFIO); else if (pcisrc->backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_XEN) - virPCIDeviceSetStubDriver(dev, VIR_PCI_STUB_DRIVER_XEN); + virPCIDeviceSetStubDriver(pci, VIR_PCI_STUB_DRIVER_XEN); else - virPCIDeviceSetStubDriver(dev, VIR_PCI_STUB_DRIVER_KVM); + virPCIDeviceSetStubDriver(pci, VIR_PCI_STUB_DRIVER_KVM); } - return list; + return pcidevs; } @@ -511,15 +511,15 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, * to pci-stub.ko */ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { - virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); + virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i); bool strict_acs_check = !!(flags & VIR_HOSTDEV_STRICT_ACS_CHECK); - bool usesVFIO = (virPCIDeviceGetStubDriver(dev) == VIR_PCI_STUB_DRIVER_VFIO); + bool usesVFIO = (virPCIDeviceGetStubDriver(pci) == VIR_PCI_STUB_DRIVER_VFIO); struct virHostdevIsPCINodeDeviceUsedData data = { mgr, dom_name, usesVFIO }; - if (!usesVFIO && !virPCIDeviceIsAssignable(dev, strict_acs_check)) { + if (!usesVFIO && !virPCIDeviceIsAssignable(pci, strict_acs_check)) { virReportError(VIR_ERR_OPERATION_INVALID, _("PCI device %s is not assignable"), - virPCIDeviceGetName(dev)); + virPCIDeviceGetName(pci)); goto cleanup; } @@ -528,7 +528,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, * belonging to same iommu group can't be shared * across guests. */ - devAddr = virPCIDeviceGetAddress(dev); + devAddr = virPCIDeviceGetAddress(pci); if (usesVFIO) { if (virPCIDeviceAddressIOMMUGroupIterate(devAddr, virHostdevIsPCINodeDeviceUsed, @@ -541,18 +541,18 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, /* Step 2: detach managed devices (i.e. bind to appropriate stub driver) */ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { - virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); + virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i); - if (virPCIDeviceGetManaged(dev)) { + if (virPCIDeviceGetManaged(pci)) { VIR_DEBUG("Detaching managed PCI device %s", - virPCIDeviceGetName(dev)); - if (virPCIDeviceDetach(dev, + virPCIDeviceGetName(pci)); + if (virPCIDeviceDetach(pci, mgr->activePCIHostdevs, mgr->inactivePCIHostdevs) < 0) goto reattachdevs; } else { VIR_DEBUG("Not detaching unmanaged PCI device %s", - virPCIDeviceGetName(dev)); + virPCIDeviceGetName(pci)); } } @@ -562,10 +562,10 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, /* Step 3: Now that all the PCI hostdevs have been detached, we * can safely reset them */ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { - virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); + virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i); - VIR_DEBUG("Resetting PCI device %s", virPCIDeviceGetName(dev)); - if (virPCIDeviceReset(dev, mgr->activePCIHostdevs, + VIR_DEBUG("Resetting PCI device %s", virPCIDeviceGetName(pci)); + if (virPCIDeviceReset(pci, mgr->activePCIHostdevs, mgr->inactivePCIHostdevs) < 0) goto reattachdevs; } @@ -585,41 +585,41 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, /* Step 5: Now mark all the devices as active */ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { - virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); + virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i); VIR_DEBUG("Adding PCI device %s to active list", - virPCIDeviceGetName(dev)); - if (virPCIDeviceListAdd(mgr->activePCIHostdevs, dev) < 0) + virPCIDeviceGetName(pci)); + if (virPCIDeviceListAdd(mgr->activePCIHostdevs, pci) < 0) goto inactivedevs; } /* Step 6: Now remove the devices from inactive list. */ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { - virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); + virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i); VIR_DEBUG("Removing PCI device %s from inactive list", - virPCIDeviceGetName(dev)); - virPCIDeviceListDel(mgr->inactivePCIHostdevs, dev); + virPCIDeviceGetName(pci)); + virPCIDeviceListDel(mgr->inactivePCIHostdevs, pci); } /* Step 7: Now set the used_by_domain of the device in * activePCIHostdevs as domain name. */ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { - virPCIDevicePtr dev, activeDev; + virPCIDevicePtr pci, actual; - dev = virPCIDeviceListGet(pcidevs, i); - activeDev = virPCIDeviceListFind(mgr->activePCIHostdevs, dev); + pci = virPCIDeviceListGet(pcidevs, i); + actual = virPCIDeviceListFind(mgr->activePCIHostdevs, pci); VIR_DEBUG("Setting driver and domain information for PCI device %s", - virPCIDeviceGetName(dev)); - if (activeDev) - virPCIDeviceSetUsedBy(activeDev, drv_name, dom_name); + virPCIDeviceGetName(pci)); + if (actual) + virPCIDeviceSetUsedBy(actual, drv_name, dom_name); } /* Step 8: Now set the original states for hostdev def */ for (i = 0; i < nhostdevs; i++) { - virPCIDevicePtr dev; + virPCIDevicePtr pci; virDomainHostdevDefPtr hostdev = hostdevs[i]; virDomainHostdevSubsysPCIPtr pcisrc = &hostdev->source.subsys.u.pci; @@ -628,7 +628,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) continue; - dev = virPCIDeviceListFindByIDs(pcidevs, + pci = virPCIDeviceListFindByIDs(pcidevs, pcisrc->addr.domain, pcisrc->addr.bus, pcisrc->addr.slot, @@ -637,15 +637,15 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, /* Appropriate values for the unbind_from_stub, remove_slot * and reprobe properties of the device were set earlier * by virPCIDeviceDetach() */ - if (dev) { + if (pci) { VIR_DEBUG("Saving network configuration of PCI device %s", - virPCIDeviceGetName(dev)); + virPCIDeviceGetName(pci)); hostdev->origstates.states.pci.unbind_from_stub = - virPCIDeviceGetUnbindFromStub(dev); + virPCIDeviceGetUnbindFromStub(pci); hostdev->origstates.states.pci.remove_slot = - virPCIDeviceGetRemoveSlot(dev); + virPCIDeviceGetRemoveSlot(pci); hostdev->origstates.states.pci.reprobe = - virPCIDeviceGetReprobe(dev); + virPCIDeviceGetReprobe(pci); } } @@ -661,11 +661,11 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, * free them in virObjectUnref(). */ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { - virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); + virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i); VIR_DEBUG("Removing PCI device %s from active list", - virPCIDeviceGetName(dev)); - virPCIDeviceListSteal(mgr->activePCIHostdevs, dev); + virPCIDeviceGetName(pci)); + virPCIDeviceListSteal(mgr->activePCIHostdevs, pci); } resetvfnetconfig: @@ -674,17 +674,17 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, reattachdevs: for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { - virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); + virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i); - if (virPCIDeviceGetManaged(dev)) { + if (virPCIDeviceGetManaged(pci)) { VIR_DEBUG("Reattaching managed PCI device %s", - virPCIDeviceGetName(dev)); - ignore_value(virPCIDeviceReattach(dev, + virPCIDeviceGetName(pci)); + ignore_value(virPCIDeviceReattach(pci, mgr->activePCIHostdevs, mgr->inactivePCIHostdevs)); } else { VIR_DEBUG("Not reattaching unmanaged PCI device %s", - virPCIDeviceGetName(dev)); + virPCIDeviceGetName(pci)); } } @@ -702,38 +702,38 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, */ static void virHostdevReattachPCIDevice(virHostdevManagerPtr mgr, - virPCIDevicePtr dev) + virPCIDevicePtr actual) { /* If the device is not managed and was attached to guest * successfully, it must have been inactive. */ - if (!virPCIDeviceGetManaged(dev)) { + if (!virPCIDeviceGetManaged(actual)) { VIR_DEBUG("Adding unmanaged PCI device %s to inactive list", - virPCIDeviceGetName(dev)); - if (virPCIDeviceListAdd(mgr->inactivePCIHostdevs, dev) < 0) - virPCIDeviceFree(dev); + virPCIDeviceGetName(actual)); + if (virPCIDeviceListAdd(mgr->inactivePCIHostdevs, actual) < 0) + virPCIDeviceFree(actual); return; } /* Wait for device cleanup if it is qemu/kvm */ - if (virPCIDeviceGetStubDriver(dev) == VIR_PCI_STUB_DRIVER_KVM) { + if (virPCIDeviceGetStubDriver(actual) == VIR_PCI_STUB_DRIVER_KVM) { int retries = 100; - while (virPCIDeviceWaitForCleanup(dev, "kvm_assigned_device") + while (virPCIDeviceWaitForCleanup(actual, "kvm_assigned_device") && retries) { usleep(100*1000); retries--; } } - VIR_DEBUG("Reattaching PCI device %s", virPCIDeviceGetName(dev)); - if (virPCIDeviceReattach(dev, mgr->activePCIHostdevs, + VIR_DEBUG("Reattaching PCI device %s", virPCIDeviceGetName(actual)); + if (virPCIDeviceReattach(actual, mgr->activePCIHostdevs, mgr->inactivePCIHostdevs) < 0) { virErrorPtr err = virGetLastError(); VIR_ERROR(_("Failed to re-attach PCI device: %s"), err ? err->message : _("unknown error")); virResetError(err); } - virPCIDeviceFree(dev); + virPCIDeviceFree(actual); } /* @oldStateDir: @@ -772,28 +772,28 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr mgr, */ i = 0; while (i < virPCIDeviceListCount(pcidevs)) { - virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); - virPCIDevicePtr activeDev = NULL; - - activeDev = virPCIDeviceListFind(mgr->activePCIHostdevs, dev); - if (activeDev) { - const char *usedby_drvname; - const char *usedby_domname; - virPCIDeviceGetUsedBy(activeDev, &usedby_drvname, &usedby_domname); - if (STRNEQ_NULLABLE(drv_name, usedby_drvname) || - STRNEQ_NULLABLE(dom_name, usedby_domname)) { - - virPCIDeviceListDel(pcidevs, dev); + virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i); + virPCIDevicePtr actual = NULL; + + actual = virPCIDeviceListFind(mgr->activePCIHostdevs, pci); + if (actual) { + const char *actual_drvname; + const char *actual_domname; + virPCIDeviceGetUsedBy(actual, &actual_drvname, &actual_domname); + if (STRNEQ_NULLABLE(drv_name, actual_drvname) || + STRNEQ_NULLABLE(dom_name, actual_domname)) { + + virPCIDeviceListDel(pcidevs, pci); continue; } } else { - virPCIDeviceListDel(pcidevs, dev); + virPCIDeviceListDel(pcidevs, pci); continue; } VIR_DEBUG("Removing PCI device %s from active list", - virPCIDeviceGetName(dev)); - virPCIDeviceListDel(mgr->activePCIHostdevs, dev); + virPCIDeviceGetName(pci)); + virPCIDeviceListDel(mgr->activePCIHostdevs, pci); i++; } @@ -809,17 +809,17 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr mgr, if (virHostdevIsPCINetDevice(hostdev)) { virDomainHostdevSubsysPCIPtr pcisrc = &hostdev->source.subsys.u.pci; - virPCIDevicePtr dev; + virPCIDevicePtr pci; - dev = virPCIDeviceListFindByIDs(pcidevs, + pci = virPCIDeviceListFindByIDs(pcidevs, pcisrc->addr.domain, pcisrc->addr.bus, pcisrc->addr.slot, pcisrc->addr.function); - if (dev) { + if (pci) { VIR_DEBUG("Restoring network configuration of PCI device %s", - virPCIDeviceGetName(dev)); + virPCIDeviceGetName(pci)); virHostdevNetConfigRestore(hostdev, mgr->stateDir, oldStateDir); } @@ -828,10 +828,10 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr mgr, /* Step 3: perform a PCI Reset on all devices */ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { - virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); + virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i); - VIR_DEBUG("Resetting PCI device %s", virPCIDeviceGetName(dev)); - if (virPCIDeviceReset(dev, mgr->activePCIHostdevs, + VIR_DEBUG("Resetting PCI device %s", virPCIDeviceGetName(pci)); + if (virPCIDeviceReset(pci, mgr->activePCIHostdevs, mgr->inactivePCIHostdevs) < 0) { virErrorPtr err = virGetLastError(); VIR_ERROR(_("Failed to reset PCI device: %s"), @@ -844,8 +844,8 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr mgr, * them on the inactive list (if not managed) */ while (virPCIDeviceListCount(pcidevs) > 0) { - virPCIDevicePtr dev = virPCIDeviceListStealIndex(pcidevs, 0); - virHostdevReattachPCIDevice(mgr, dev); + virPCIDevicePtr pci = virPCIDeviceListStealIndex(pcidevs, 0); + virHostdevReattachPCIDevice(mgr, pci); } virObjectUnref(pcidevs); @@ -862,7 +862,7 @@ virHostdevUpdateActivePCIDevices(virHostdevManagerPtr mgr, const char *dom_name) { virDomainHostdevDefPtr hostdev = NULL; - virPCIDevicePtr dev = NULL; + virPCIDevicePtr actual = NULL; size_t i; int ret = -1; @@ -882,35 +882,35 @@ virHostdevUpdateActivePCIDevices(virHostdevManagerPtr mgr, if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) continue; - dev = virPCIDeviceNew(pcisrc->addr.domain, pcisrc->addr.bus, - pcisrc->addr.slot, pcisrc->addr.function); + actual = virPCIDeviceNew(pcisrc->addr.domain, pcisrc->addr.bus, + pcisrc->addr.slot, pcisrc->addr.function); - if (!dev) + if (!actual) goto cleanup; - virPCIDeviceSetManaged(dev, hostdev->managed); - virPCIDeviceSetUsedBy(dev, drv_name, dom_name); + virPCIDeviceSetManaged(actual, hostdev->managed); + virPCIDeviceSetUsedBy(actual, drv_name, dom_name); if (pcisrc->backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) - virPCIDeviceSetStubDriver(dev, VIR_PCI_STUB_DRIVER_VFIO); + virPCIDeviceSetStubDriver(actual, VIR_PCI_STUB_DRIVER_VFIO); else if (pcisrc->backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_XEN) - virPCIDeviceSetStubDriver(dev, VIR_PCI_STUB_DRIVER_XEN); + virPCIDeviceSetStubDriver(actual, VIR_PCI_STUB_DRIVER_XEN); else - virPCIDeviceSetStubDriver(dev, VIR_PCI_STUB_DRIVER_KVM); + virPCIDeviceSetStubDriver(actual, VIR_PCI_STUB_DRIVER_KVM); /* Setup the original states for the PCI device */ - virPCIDeviceSetUnbindFromStub(dev, hostdev->origstates.states.pci.unbind_from_stub); - virPCIDeviceSetRemoveSlot(dev, hostdev->origstates.states.pci.remove_slot); - virPCIDeviceSetReprobe(dev, hostdev->origstates.states.pci.reprobe); + virPCIDeviceSetUnbindFromStub(actual, hostdev->origstates.states.pci.unbind_from_stub); + virPCIDeviceSetRemoveSlot(actual, hostdev->origstates.states.pci.remove_slot); + virPCIDeviceSetReprobe(actual, hostdev->origstates.states.pci.reprobe); - if (virPCIDeviceListAdd(mgr->activePCIHostdevs, dev) < 0) + if (virPCIDeviceListAdd(mgr->activePCIHostdevs, actual) < 0) goto cleanup; - dev = NULL; + actual = NULL; } ret = 0; cleanup: - virPCIDeviceFree(dev); + virPCIDeviceFree(actual); virObjectUnlock(mgr->activePCIHostdevs); virObjectUnlock(mgr->inactivePCIHostdevs); return ret; -- 2.5.0
On 03/07/2016 12:24 PM, Andrea Bolognani wrote:
This is not just a cosmetic change: the name of the variable now gives a hint about what it is supposed to be used for. --- src/util/virhostdev.c | 234 +++++++++++++++++++++++++------------------------- 1 file changed, 117 insertions(+), 117 deletions(-)
After a brief return to patch 15... ACK, John
These comments explain the difference between a virPCIDevice instance used for lookups and an actual device instance; some information is also provided for specific uses. --- src/util/virhostdev.c | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index 10d1c1a..a431f0a 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -60,6 +60,27 @@ struct virHostdevIsPCINodeDeviceUsedData { const bool usesVFIO; }; +/* This module makes heavy use of bookkeeping lists, contained inside a + * virHostdevManager instance, to keep track of the devices' status. To make + * it easy to spot potential ownership errors when moving devices from one + * list to the other, variable names should comply with the following + * conventions when it comes to virPCIDevice and virPCIDeviceList instances: + * + * pci - a short-lived virPCIDevice whose purpose is usually just to look + * up the actual PCI device in one of the bookkeeping lists; basically + * little more than a fancy virPCIDeviceAddress + * + * pcidevs - a list containing a bunch of the above + * + * actual - a virPCIDevice instance that has either been retrieved from one + * of the bookkeeping lists, or is intended to be added or copied + * to one at some point + * + * Passing an 'actual' to a function that requires a 'pci' is fine, but the + * opposite is usually not true; as a rule of thumb, functions in the virpci + * module usually expect an 'actual'. Even with these conventions in place, + * adding comments to highlight ownership-related issues is recommended */ + static int virHostdevIsPCINodeDeviceUsed(virPCIDeviceAddressPtr devAddr, void *opaque) { virPCIDevicePtr actual; @@ -544,6 +565,10 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i); if (virPCIDeviceGetManaged(pci)) { + /* We can't look up the actual device because it has not been + * created yet: virPCIDeviceDetach() will insert a copy of 'pci' + * into the list of inactive devices, and that copy will be the + * actual device going forward */ VIR_DEBUG("Detaching managed PCI device %s", virPCIDeviceGetName(pci)); if (virPCIDeviceDetach(pci, @@ -564,6 +589,9 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i); + /* We can avoid looking up the actual device here, because performing + * a PCI reset on a device doesn't require any information other than + * the address, which 'pci' already contains */ VIR_DEBUG("Resetting PCI device %s", virPCIDeviceGetName(pci)); if (virPCIDeviceReset(pci, mgr->activePCIHostdevs, mgr->inactivePCIHostdevs) < 0) @@ -608,6 +636,9 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr pci, actual; + /* We need to look up the actual device and set the information + * there because 'pci' only contain address information and will + * be released at the end of the function */ pci = virPCIDeviceListGet(pcidevs, i); actual = virPCIDeviceListFind(mgr->activePCIHostdevs, pci); @@ -775,6 +806,10 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr mgr, virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i); virPCIDevicePtr actual = NULL; + /* We need to look up the actual device, which is the one containing + * information such as by which domain and driver it is used. As a + * side effect, by looking it up we can also tell whether it was + * really active in the first place */ actual = virPCIDeviceListFind(mgr->activePCIHostdevs, pci); if (actual) { const char *actual_drvname; @@ -830,6 +865,9 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr mgr, for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i); + /* We can avoid looking up the actual device here, because performing + * a PCI reset on a device doesn't require any information other than + * the address, which 'pci' already contains */ VIR_DEBUG("Resetting PCI device %s", virPCIDeviceGetName(pci)); if (virPCIDeviceReset(pci, mgr->activePCIHostdevs, mgr->inactivePCIHostdevs) < 0) { -- 2.5.0
On 03/07/2016 12:24 PM, Andrea Bolognani wrote:
These comments explain the difference between a virPCIDevice instance used for lookups and an actual device instance; some information is also provided for specific uses. --- src/util/virhostdev.c | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+)
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index 10d1c1a..a431f0a 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -60,6 +60,27 @@ struct virHostdevIsPCINodeDeviceUsedData { const bool usesVFIO; };
+/* This module makes heavy use of bookkeeping lists, contained inside a ^ no comma.
+ * virHostdevManager instance, to keep track of the devices' status. To make ^ same
+ * it easy to spot potential ownership errors when moving devices from one + * list to the other, variable names should comply with the following + * conventions when it comes to virPCIDevice and virPCIDeviceList instances: + * + * pci - a short-lived virPCIDevice whose purpose is usually just to look + * up the actual PCI device in one of the bookkeeping lists; basically + * little more than a fancy virPCIDeviceAddress + * + * pcidevs - a list containing a bunch of the above + * + * actual - a virPCIDevice instance that has either been retrieved from one + * of the bookkeeping lists, or is intended to be added or copied + * to one at some point + * + * Passing an 'actual' to a function that requires a 'pci' is fine, but the + * opposite is usually not true; as a rule of thumb, functions in the virpci + * module usually expect an 'actual'. Even with these conventions in place, + * adding comments to highlight ownership-related issues is recommended */ +
s//Free beer for anyone that reads this and adheres to it. You will need it./
static int virHostdevIsPCINodeDeviceUsed(virPCIDeviceAddressPtr devAddr, void *opaque) { virPCIDevicePtr actual; @@ -544,6 +565,10 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i);
if (virPCIDeviceGetManaged(pci)) {
Just for consistency/readability - add an extra line between the if and comment open. ACK John
+ /* We can't look up the actual device because it has not been + * created yet: virPCIDeviceDetach() will insert a copy of 'pci' + * into the list of inactive devices, and that copy will be the + * actual device going forward */ VIR_DEBUG("Detaching managed PCI device %s", virPCIDeviceGetName(pci)); if (virPCIDeviceDetach(pci, @@ -564,6 +589,9 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i);
+ /* We can avoid looking up the actual device here, because performing + * a PCI reset on a device doesn't require any information other than + * the address, which 'pci' already contains */ VIR_DEBUG("Resetting PCI device %s", virPCIDeviceGetName(pci)); if (virPCIDeviceReset(pci, mgr->activePCIHostdevs, mgr->inactivePCIHostdevs) < 0) @@ -608,6 +636,9 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr pci, actual;
+ /* We need to look up the actual device and set the information + * there because 'pci' only contain address information and will + * be released at the end of the function */ pci = virPCIDeviceListGet(pcidevs, i); actual = virPCIDeviceListFind(mgr->activePCIHostdevs, pci);
@@ -775,6 +806,10 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr mgr, virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i); virPCIDevicePtr actual = NULL;
+ /* We need to look up the actual device, which is the one containing + * information such as by which domain and driver it is used. As a + * side effect, by looking it up we can also tell whether it was + * really active in the first place */ actual = virPCIDeviceListFind(mgr->activePCIHostdevs, pci); if (actual) { const char *actual_drvname; @@ -830,6 +865,9 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr mgr, for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i);
+ /* We can avoid looking up the actual device here, because performing + * a PCI reset on a device doesn't require any information other than + * the address, which 'pci' already contains */ VIR_DEBUG("Resetting PCI device %s", virPCIDeviceGetName(pci)); if (virPCIDeviceReset(pci, mgr->activePCIHostdevs, mgr->inactivePCIHostdevs) < 0) {
We might be just fine looking up the information in pcidevs, but it wouldn't save us any trouble and it's better to be consistent. --- src/util/virhostdev.c | 41 ++++++++++++++++++++++------------------- 1 file changed, 22 insertions(+), 19 deletions(-) diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index a431f0a..03c3445 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -650,7 +650,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, /* Step 8: Now set the original states for hostdev def */ for (i = 0; i < nhostdevs; i++) { - virPCIDevicePtr pci; + virPCIDevicePtr actual; virDomainHostdevDefPtr hostdev = hostdevs[i]; virDomainHostdevSubsysPCIPtr pcisrc = &hostdev->source.subsys.u.pci; @@ -659,24 +659,27 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) continue; - pci = virPCIDeviceListFindByIDs(pcidevs, - pcisrc->addr.domain, - pcisrc->addr.bus, - pcisrc->addr.slot, - pcisrc->addr.function); + /* We need to look up the actual device because it's the one + * that contains the information we care about (unbind_from_stub, + * remove_slot, reprobe) */ + actual = virPCIDeviceListFindByIDs(mgr->activePCIHostdevs, + pcisrc->addr.domain, + pcisrc->addr.bus, + pcisrc->addr.slot, + pcisrc->addr.function); /* Appropriate values for the unbind_from_stub, remove_slot * and reprobe properties of the device were set earlier * by virPCIDeviceDetach() */ - if (pci) { + if (actual) { VIR_DEBUG("Saving network configuration of PCI device %s", - virPCIDeviceGetName(pci)); + virPCIDeviceGetName(actual)); hostdev->origstates.states.pci.unbind_from_stub = - virPCIDeviceGetUnbindFromStub(pci); + virPCIDeviceGetUnbindFromStub(actual); hostdev->origstates.states.pci.remove_slot = - virPCIDeviceGetRemoveSlot(pci); + virPCIDeviceGetRemoveSlot(actual); hostdev->origstates.states.pci.reprobe = - virPCIDeviceGetReprobe(pci); + virPCIDeviceGetReprobe(actual); } } @@ -844,17 +847,17 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr mgr, if (virHostdevIsPCINetDevice(hostdev)) { virDomainHostdevSubsysPCIPtr pcisrc = &hostdev->source.subsys.u.pci; - virPCIDevicePtr pci; + virPCIDevicePtr actual; - pci = virPCIDeviceListFindByIDs(pcidevs, - pcisrc->addr.domain, - pcisrc->addr.bus, - pcisrc->addr.slot, - pcisrc->addr.function); + actual = virPCIDeviceListFindByIDs(mgr->inactivePCIHostdevs, + pcisrc->addr.domain, + pcisrc->addr.bus, + pcisrc->addr.slot, + pcisrc->addr.function); - if (pci) { + if (actual) { VIR_DEBUG("Restoring network configuration of PCI device %s", - virPCIDeviceGetName(pci)); + virPCIDeviceGetName(actual)); virHostdevNetConfigRestore(hostdev, mgr->stateDir, oldStateDir); } -- 2.5.0
On 03/07/2016 12:24 PM, Andrea Bolognani wrote:
We might be just fine looking up the information in pcidevs, but it wouldn't save us any trouble and it's better to be consistent. --- src/util/virhostdev.c | 41 ++++++++++++++++++++++------------------- 1 file changed, 22 insertions(+), 19 deletions(-)
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index a431f0a..03c3445 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -650,7 +650,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr,
/* Step 8: Now set the original states for hostdev def */ for (i = 0; i < nhostdevs; i++) { - virPCIDevicePtr pci; + virPCIDevicePtr actual; virDomainHostdevDefPtr hostdev = hostdevs[i]; virDomainHostdevSubsysPCIPtr pcisrc = &hostdev->source.subsys.u.pci;
@@ -659,24 +659,27 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) continue;
- pci = virPCIDeviceListFindByIDs(pcidevs, - pcisrc->addr.domain, - pcisrc->addr.bus, - pcisrc->addr.slot, - pcisrc->addr.function); + /* We need to look up the actual device because it's the one + * that contains the information we care about (unbind_from_stub, + * remove_slot, reprobe) */
When a device goes from the inactivePCIHostdevs list to the activePCIHostdevs list "at some point in time" in the future - does it do a similar save? That is this change only grabs devices from the active list for the save; whereas, prior to this change it would pull from all pcidevs
+ actual = virPCIDeviceListFindByIDs(mgr->activePCIHostdevs, + pcisrc->addr.domain, + pcisrc->addr.bus, + pcisrc->addr.slot, + pcisrc->addr.function);
/* Appropriate values for the unbind_from_stub, remove_slot * and reprobe properties of the device were set earlier * by virPCIDeviceDetach() */ - if (pci) { + if (actual) { VIR_DEBUG("Saving network configuration of PCI device %s", - virPCIDeviceGetName(pci)); + virPCIDeviceGetName(actual)); hostdev->origstates.states.pci.unbind_from_stub = - virPCIDeviceGetUnbindFromStub(pci); + virPCIDeviceGetUnbindFromStub(actual); hostdev->origstates.states.pci.remove_slot = - virPCIDeviceGetRemoveSlot(pci); + virPCIDeviceGetRemoveSlot(actual); hostdev->origstates.states.pci.reprobe = - virPCIDeviceGetReprobe(pci); + virPCIDeviceGetReprobe(actual); } }
@@ -844,17 +847,17 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr mgr,
if (virHostdevIsPCINetDevice(hostdev)) { virDomainHostdevSubsysPCIPtr pcisrc = &hostdev->source.subsys.u.pci; - virPCIDevicePtr pci; + virPCIDevicePtr actual;
- pci = virPCIDeviceListFindByIDs(pcidevs, - pcisrc->addr.domain, - pcisrc->addr.bus, - pcisrc->addr.slot, - pcisrc->addr.function);
So the previous loop took care of the activePCIHostdevs, correct? And this loop takes care of the inactivePCIHostdevs for reattachement? I think this part is correct - although is it worthy of splitting into it's own separate patch? John Going to stop here for now (we have company).
+ actual = virPCIDeviceListFindByIDs(mgr->inactivePCIHostdevs, + pcisrc->addr.domain, + pcisrc->addr.bus, + pcisrc->addr.slot, + pcisrc->addr.function);
- if (pci) { + if (actual) { VIR_DEBUG("Restoring network configuration of PCI device %s", - virPCIDeviceGetName(pci)); + virPCIDeviceGetName(actual)); virHostdevNetConfigRestore(hostdev, mgr->stateDir, oldStateDir); }
On 03/10/2016 06:02 PM, John Ferlan wrote:
On 03/07/2016 12:24 PM, Andrea Bolognani wrote:
We might be just fine looking up the information in pcidevs, but it wouldn't save us any trouble and it's better to be consistent. --- src/util/virhostdev.c | 41 ++++++++++++++++++++++------------------- 1 file changed, 22 insertions(+), 19 deletions(-)
So I went through this one again (new day, fresh start, partially clear mind).
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index a431f0a..03c3445 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c
First off - perhaps something for the previous documentation patch - PrepareDevices is called during the qemuProcessLaunch processing (eg domain startup) for all known host devices. It's also called during qemuDomainAttachHostPCIDevice or libxlDomainAttachHostPCIDevice (hotplug) for the *one* new device to be attached. The purpose of the function is to take the passed hostdev list, move the devices to a managed active host device list for the guest. Devices that do not have the managed attribute are not processed.
@@ -650,7 +650,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr,
So after step 6, all the pcidevs are on the activePCIHostdevs list. Theoretically at least ;-). None are on the inactivePCIHostdevs list. Why in step 7 do we run through pcidevs, to find the device on the activePCIHostdev list in order to set the UsedBy of the actual device on the active list. Why not run the 'activePCIHostdev' list here too? Since we really don't need pcidevs any more - can we delete it now? E.G. Step 9 it... Steps 7 & 8 don't really need pcidevs, right? There is no error path for either. I'm not asking for a patch - just making sure I'm reading things right!
/* Step 8: Now set the original states for hostdev def */ for (i = 0; i < nhostdevs; i++) { - virPCIDevicePtr pci; + virPCIDevicePtr actual; virDomainHostdevDefPtr hostdev = hostdevs[i]; virDomainHostdevSubsysPCIPtr pcisrc = &hostdev->source.subsys.u.pci;
@@ -659,24 +659,27 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) continue;
- pci = virPCIDeviceListFindByIDs(pcidevs, - pcisrc->addr.domain, - pcisrc->addr.bus, - pcisrc->addr.slot, - pcisrc->addr.function); + /* We need to look up the actual device because it's the one + * that contains the information we care about (unbind_from_stub, + * remove_slot, reprobe) */
When a device goes from the inactivePCIHostdevs list to the activePCIHostdevs list "at some point in time" in the future - does it do a similar save?
I'll answer my own question - because this is the Prepare function and we don't have to worry about it since everything is on the active list.
That is this change only grabs devices from the active list for the save; whereas, prior to this change it would pull from all pcidevs
So, I believe this part is correct albeit a bit confusing...
+ actual = virPCIDeviceListFindByIDs(mgr->activePCIHostdevs, + pcisrc->addr.domain, + pcisrc->addr.bus, + pcisrc->addr.slot, + pcisrc->addr.function);
/* Appropriate values for the unbind_from_stub, remove_slot * and reprobe properties of the device were set earlier * by virPCIDeviceDetach() */ - if (pci) { + if (actual) { VIR_DEBUG("Saving network configuration of PCI device %s", - virPCIDeviceGetName(pci)); + virPCIDeviceGetName(actual)); hostdev->origstates.states.pci.unbind_from_stub = - virPCIDeviceGetUnbindFromStub(pci); + virPCIDeviceGetUnbindFromStub(actual); hostdev->origstates.states.pci.remove_slot = - virPCIDeviceGetRemoveSlot(pci); + virPCIDeviceGetRemoveSlot(actual); hostdev->origstates.states.pci.reprobe = - virPCIDeviceGetReprobe(pci); + virPCIDeviceGetReprobe(actual); } }
@@ -844,17 +847,17 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr mgr,
More documentation thoughts... Because what exists now for the function doesn't really make sense - it's only describing one variable. Anyway, the purpose of this function is to take the device(s) passed on the hostdev list and return them to the host (?if they are managed?). This function is called either from hotplug device remove, hotplug device attach failure path, or from qemuProcessStop during process shutdown. When called from the host device remove or attach failure paths, there is only 1 hostdev in the list. ... Prior to patch 15, the pcidev's was a copy of the "active list", now it's a list of all pcidevs found. Hopefully on either one of the two lists. That is I hope a found hostdev couldn't be on neither list. Prior to patch 15, step1 would either remove the device from pcidevs or from the activeList depending on the matching driver name. When done, the pcidevs would contain the list of devices just removed from the activeList. Note that virPCIDeviceListDel will repeat the active list virPCIDeviceListFind (essentially)... After patch 15, the pcidevs gets reduced for devices not on the activeList or without a matching drv/dom name. Whether or not that is on the inactiveList isn't checked, but assumed. Step 2 is I believe the antecedent of step 4 during the Prepare path. Step 4 would be called after Reset and before placing devices on active list replacing the net config for any PCINet device... Prior to patch 15, step2 would walk the hostdev's list looking for devices on the pcidevs list (eg. the ones removed from the activeList). It would then take any PCINet device and restore it's config. After patch 15 and with these changes, walk the inactiveList and perform the same call. This is not the devices we just removed from the activeList, so I don't think using the inactiveList in this case is right. Before patch 15, step 3 would take any of the devices we removed from the activeList and perform a reset on them. After patch 15, step 3 would do a similar function assuming that no device could not be on neither list. Hopefully this all makes sense - I keep reading it and going back and fort between old and new code. The comment left in the code after step 1 is most helpful... John
if (virHostdevIsPCINetDevice(hostdev)) { virDomainHostdevSubsysPCIPtr pcisrc = &hostdev->source.subsys.u.pci; - virPCIDevicePtr pci; + virPCIDevicePtr actual;
- pci = virPCIDeviceListFindByIDs(pcidevs, - pcisrc->addr.domain, - pcisrc->addr.bus, - pcisrc->addr.slot, - pcisrc->addr.function);
So the previous loop took care of the activePCIHostdevs, correct? And this loop takes care of the inactivePCIHostdevs for reattachement? I think this part is correct - although is it worthy of splitting into it's own separate patch?
John
Going to stop here for now (we have company).
+ actual = virPCIDeviceListFindByIDs(mgr->inactivePCIHostdevs, + pcisrc->addr.domain, + pcisrc->addr.bus, + pcisrc->addr.slot, + pcisrc->addr.function);
- if (pci) { + if (actual) { VIR_DEBUG("Restoring network configuration of PCI device %s", - virPCIDeviceGetName(pci)); + virPCIDeviceGetName(actual)); virHostdevNetConfigRestore(hostdev, mgr->stateDir, oldStateDir); }
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Word of warning, since this is very likely to get extremely verbose: I fully expect to slip, here and there, and refer not to the *current* situation as of this patch, but to the *desired* situation at the end of the series. Such *desired* situation is (not explicitly enough?) laid out in patch 19, but I'll expand on it here for clarity: * the active list and inactive list contain the actual devices * each device is contained either in the active list, or in the inactive list. It can't be in both * a device that's neither in the active list nor in the inactive list is assigned to the host * for each PCI device, only the virPCIDevice instance that is part of one of the bookkeeping lists matter * when an operation is to be performed on a PCI device, the relevant virPCIDevice instance must be looked up in the appropriate bookkeeping list * virPCIDevice instances in 'pcidevs' are only used for looking up actual devices in the bookkeeping lists, contain no valuable data and are disposable at any time * exceptions to the above points are to be documented Hopefully this makes it easier to see the meaning behind some of the changes :) On Fri, 2016-03-11 at 14:40 -0500, John Ferlan wrote:
On 03/10/2016 06:02 PM, John Ferlan wrote:
On 03/07/2016 12:24 PM, Andrea Bolognani wrote: So I went through this one again (new day, fresh start, partially clear mind).
That often helps :)
First off - perhaps something for the previous documentation patch - PrepareDevices is called during the qemuProcessLaunch processing (eg domain startup) for all known host devices. It's also called during qemuDomainAttachHostPCIDevice or libxlDomainAttachHostPCIDevice (hotplug) for the *one* new device to be attached. The purpose of the function is to take the passed hostdev list, move the devices to a managed active host device list for the guest. Devices that do not have the managed attribute are not processed.
That's not true, though: unmanaged devices *are* processed, even if less operations are performed on them. The part about detaching them from the host is skipped, because it's supposed to have been performed by the user alread, but that's it; everything else (resetting them, moving them to the appropriate bookkeeping list, storing information about what driver and domain is using them) is just the same as managed devices.
@@ -650,7 +650,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr,
So after step 6, all the pcidevs are on the activePCIHostdevs list. Theoretically at least ;-). None are on the inactivePCIHostdevs list. Why in step 7 do we run through pcidevs, to find the device on the activePCIHostdev list in order to set the UsedBy of the actual device on the active list. Why not run the 'activePCIHostdev' list here too? Since we really don't need pcidevs any more - can we delete it now? E.G. Step 9 it... Steps 7 & 8 don't really need pcidevs, right? There is no error path for either. I'm not asking for a patch - just making sure I'm reading things right!
I guess we *could* get rid of it early (even though I haven't really checked to make sure it would work), but doing so wouldn't give us any advantage IMHO. As it is now (well, not as of patch 20, but as of patch 22 which is basically what this whole series is building towards) 'pcidevs' is used only for iteration, and iteration is performed only over 'pcidevs'. This is exactly the kind of straightforward knowledge we need to hold on to if we are ever to get this code in a manageable state. Examples of stuff that is easier to reason about if we keep 'pcidevs' around and only ever iterate over it: to obtain an actual device, we *always* need to look it up in the appropriate list, no ambiguity; in the cleanup path, we can *always* just destroy all of 'pcidevs', because no valuable data is stored in there. So, even if it might mean performing a couple more lookups here and there, I think the separation between 'pcidevs' (a list of instances we can use to look up the actual data) and the actual bookkeeping lists (where the data we care about is stored) is crucial and should be enforced.
/* Step 8: Now set the original states for hostdev def */ for (i = 0; i < nhostdevs; i++) { - virPCIDevicePtr pci; + virPCIDevicePtr actual; virDomainHostdevDefPtr hostdev = hostdevs[i]; virDomainHostdevSubsysPCIPtr pcisrc = &hostdev->source.subsys.u.pci; @@ -659,24 +659,27 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) continue; - pci = virPCIDeviceListFindByIDs(pcidevs, - pcisrc->addr.domain, - pcisrc->addr.bus, - pcisrc->addr.slot, - pcisrc->addr.function); + /* We need to look up the actual device because it's the one + * that contains the information we care about (unbind_from_stub, + * remove_slot, reprobe) */
When a device goes from the inactivePCIHostdevs list to the activePCIHostdevs list "at some point in time" in the future - does it do a similar save? I'll answer my own question - because this is the Prepare function and we don't have to worry about it since everything is on the active list.
Exactly, all devices that go from the inactive list to the active list do so via this function... We don't need to handle this anywhere else. And by the time this specific lookup is performed, all devices have already been moved to the active list (step 5).
That is this change only grabs devices from the active list for the save; whereas, prior to this change it would pull from all pcidevs So, I believe this part is correct albeit a bit confusing...
There are *no* devices that are in 'pcidevs' but *not* in the active list by this point - again, all objects in 'pcidevs' are used (or supposed to be used) for lookup purposes only. I wonder if a better name would help making this easier to keep in mind even for someone who's looking at the code for the first time, or even myself two months from now :) What about 'tmpDevices'? 'lookupDevices'?
+ actual = virPCIDeviceListFindByIDs(mgr->activePCIHostdevs, + pcisrc->addr.domain, + pcisrc->addr.bus, + pcisrc->addr.slot, + pcisrc->addr.function); /* Appropriate values for the unbind_from_stub, remove_slot * and reprobe properties of the device were set earlier * by virPCIDeviceDetach() */ - if (pci) { + if (actual) { VIR_DEBUG("Saving network configuration of PCI device %s", - virPCIDeviceGetName(pci)); + virPCIDeviceGetName(actual)); hostdev->origstates.states.pci.unbind_from_stub = - virPCIDeviceGetUnbindFromStub(pci); + virPCIDeviceGetUnbindFromStub(actual); hostdev->origstates.states.pci.remove_slot = - virPCIDeviceGetRemoveSlot(pci); + virPCIDeviceGetRemoveSlot(actual); hostdev->origstates.states.pci.reprobe = - virPCIDeviceGetReprobe(pci); + virPCIDeviceGetReprobe(actual); } } @@ -844,17 +847,17 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr mgr, More documentation thoughts... Because what exists now for the function doesn't really make sense - it's only describing one variable. Anyway, the purpose of this function is to take the device(s) passed on the hostdev list and return them to the host (?if they are managed?). This function is called either from hotplug device remove, hotplug device attach failure path, or from qemuProcessStop during process shutdown. When called from the host device remove or attach failure paths, there is only 1 hostdev in the list.
More documentation is definitely always a good thing, and this pretty much sums up what the function does :)
Prior to patch 15, the pcidev's was a copy of the "active list", now it's a list of all pcidevs found. Hopefully on either one of the two lists. That is I hope a found hostdev couldn't be on neither list.
See above: it's not a list of all PCI devices, just a list of PCI devices we're going to detach from the guest (and possibly reattach to the host) at this particular time.
Prior to patch 15, step1 would either remove the device from pcidevs or from the activeList depending on the matching driver name. When done, the pcidevs would contain the list of devices just removed from the activeList. Note that virPCIDeviceListDel will repeat the active list virPCIDeviceListFind (essentially)... After patch 15, the pcidevs gets reduced for devices not on the activeList or without a matching drv/dom name. Whether or not that is on the inactiveList isn't checked, but assumed. Step 2 is I believe the antecedent of step 4 during the Prepare path. Step 4 would be called after Reset and before placing devices on active list replacing the net config for any PCINet device... Prior to patch 15, step2 would walk the hostdev's list looking for devices on the pcidevs list (eg. the ones removed from the activeList). It would then take any PCINet device and restore it's config. After patch 15 and with these changes, walk the inactiveList and perform the same call. This is not the devices we just removed from the activeList, so I don't think using the inactiveList in this case is right.
Yeah, this is totally bogus. Looking up the device in the inactive list is the right thing to do *only after* patch 22 has been applied and step 2 is "Move devices from the active list to the inactive list". With that in place we are guaranteed that the actual device will be in the inactive list, and the code above will be correct. I believe I actually wrote this code after patch 22 and then squashed it in patch 20 on a misguided attempt to present changes in a more linear fashion :(
Before patch 15, step 3 would take any of the devices we removed from the activeList and perform a reset on them. After patch 15, step 3 would do a similar function assuming that no device could not be on neither list.
NO device could NOT be on NEITHER list... My head is spinning :) Actually, when we get here the devices are *guaranteed* to be in neither list, because we removed them from the active list in step 1. But that was the case even before patch 15, so it should not be a problem.
Hopefully this all makes sense - I keep reading it and going back and fort between old and new code. The comment left in the code after step 1 is most helpful...
I know how you feel! Thanks for sticking with it, hopefully the comments I've provided so far (especially the ones at the top of this message) are helpful in understanding the thought process behind the changes. I'll get back to you with more comments tomorrow - bet you can't wait for them! ;) Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
On 03/14/2016 02:00 PM, Andrea Bolognani wrote:
Word of warning, since this is very likely to get extremely verbose: I fully expect to slip, here and there, and refer not to the *current* situation as of this patch, but to the *desired* situation at the end of the series.
I've rebased to top of tree after your recent pushes... Seems like a good place to reset focus! I think I need a virtual whiteboard.
Such *desired* situation is (not explicitly enough?) laid out in patch 19, but I'll expand on it here for clarity:
* the active list and inactive list contain the actual devices
I understand/agree with the goal for actual devices in each list. One mental block is usage of virPCIDeviceListAdd for activeList and virPCIDeviceListAddCopy for inactiveList. The second mental block is whether these are for only managed devices (more on that later).
* each device is contained either in the active list, or in the inactive list. It can't be in both
Sure that's where you're headed, but with the current code this isn't necessarily true as a device could be on the inactiveList and activeList during patch 5, but that's the goal of patch 22.
* a device that's neither in the active list nor in the inactive list is assigned to the host
OK.. true... hopefully!
* for each PCI device, only the virPCIDevice instance that is part of one of the bookkeeping lists matter
right.
* when an operation is to be performed on a PCI device, the relevant virPCIDevice instance must be looked up in the appropriate bookkeeping list
sure, one would hope! Of course they need to be populated that way...
* virPCIDevice instances in 'pcidevs' are only used for looking up actual devices in the bookkeeping lists, contain no valuable data and are disposable at any time
This is where things start getting a bit fuzzy. I agree with the concept; however, the current implementation has a gotcha. Consider how VIR_APPEND_ELEMENT works. Initially 'pcidevs' is created as a new List, then for each 'hostdev' device, a new virPCIDevicePtr is created and added to the pcidevs list using virPCIDeviceListAdd. It's taken a bit to have VIR_APPEND_ELEMENT processing sink in for me ;-), but as I read the Prepare code it makes use of this processing model instead of creating a separate copy for the activeList.
* exceptions to the above points are to be documented
Hopefully this makes it easier to see the meaning behind some of the changes :)
On Fri, 2016-03-11 at 14:40 -0500, John Ferlan wrote:
On 03/10/2016 06:02 PM, John Ferlan wrote:
On 03/07/2016 12:24 PM, Andrea Bolognani wrote: So I went through this one again (new day, fresh start, partially clear mind).
That often helps :)
So here again, I'm trying to start fresh - go through each patch 1 by 1 so we can "agree" and move to the next one. Save a few electrons, too. Let's try to get to a point where patch22 can be further dissected.
First off - perhaps something for the previous documentation patch - PrepareDevices is called during the qemuProcessLaunch processing (eg domain startup) for all known host devices. It's also called during qemuDomainAttachHostPCIDevice or libxlDomainAttachHostPCIDevice (hotplug) for the *one* new device to be attached.
The purpose of the function is to take the passed hostdev list, move the devices to a managed active host device list for the guest. Devices that do not have the managed attribute are not processed.
That's not true, though: unmanaged devices *are* processed, even if less operations are performed on them. The part about detaching them from the host is skipped, because it's supposed to have been performed by the user alread, but that's it; everything else (resetting them, moving them to the appropriate bookkeeping list, storing information about what driver and domain is using them) is just the same as managed devices.
I think this is where the confusion for me lies. I seem to have also mistyped a bit - perhaps it's the blurred lines between current and future perfect (hah!) state. Initially an unmanaged device is not placed on the inactiveList (e.g. step2 in the PreparePCIDevices code), but it is placed on the activeList (step5). Later on, we can place an unmanaged device on the inactiveList either in virHostdevReattachPCIDevice using the "stolen" 'pcidevs' entry during virHostdevReAttachPCIDevices or virHostdevPCINodeDeviceDetach where virPCIDeviceDetach uses a copy of the device. Although patch22 removes the addition into the inactiveList if !managed (new step5 and deletion of lines in ReattachPCIDevice). Still leaves the NodeDeviceDetach path to understand from whence that request came.
@@ -650,7 +650,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr,
So after step 6, all the pcidevs are on the activePCIHostdevs list. Theoretically at least ;-). None are on the inactivePCIHostdevs list.
Why in step 7 do we run through pcidevs, to find the device on the activePCIHostdev list in order to set the UsedBy of the actual device on the active list. Why not run the 'activePCIHostdev' list here too?
Since we really don't need pcidevs any more - can we delete it now? E.G. Step 9 it... Steps 7 & 8 don't really need pcidevs, right? There is no error path for either. I'm not asking for a patch - just making sure I'm reading things right!
I guess we *could* get rid of it early (even though I haven't really checked to make sure it would work), but doing so wouldn't give us any advantage IMHO.
As it is now (well, not as of patch 20, but as of patch 22 which is basically what this whole series is building towards) 'pcidevs' is used only for iteration, and iteration is performed only over 'pcidevs'. This is exactly the kind of straightforward knowledge we need to hold on to if we are ever to get this code in a manageable state.
OK - it was a suggestion; however, as you'll see below I there's a caveat with this pcidevs list and the activeList...
Examples of stuff that is easier to reason about if we keep 'pcidevs' around and only ever iterate over it: to obtain an actual device, we *always* need to look it up in the appropriate list, no ambiguity; in the cleanup path, we can *always* just destroy all of 'pcidevs', because no valuable data is stored in there.
So, even if it might mean performing a couple more lookups here and there, I think the separation between 'pcidevs' (a list of instances we can use to look up the actual data) and the actual bookkeeping lists (where the data we care about is stored) is crucial and should be enforced.
OK, understood - I agree. The extra lookups though can be expensive, but safety first.
/* Step 8: Now set the original states for hostdev def */ for (i = 0; i < nhostdevs; i++) { - virPCIDevicePtr pci; + virPCIDevicePtr actual; virDomainHostdevDefPtr hostdev = hostdevs[i]; virDomainHostdevSubsysPCIPtr pcisrc = &hostdev->source.subsys.u.pci;
@@ -659,24 +659,27 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) continue;
- pci = virPCIDeviceListFindByIDs(pcidevs, - pcisrc->addr.domain, - pcisrc->addr.bus, - pcisrc->addr.slot, - pcisrc->addr.function); + /* We need to look up the actual device because it's the one + * that contains the information we care about (unbind_from_stub, + * remove_slot, reprobe) */
When a device goes from the inactivePCIHostdevs list to the activePCIHostdevs list "at some point in time" in the future - does it do a similar save?
I'll answer my own question - because this is the Prepare function and we don't have to worry about it since everything is on the active list.
Exactly, all devices that go from the inactive list to the active list do so via this function... We don't need to handle this anywhere else. And by the time this specific lookup is performed, all devices have already been moved to the active list (step 5).
Right, but if I read patch22 correctly (jumping slightly ahead), only managed devices would be placed onto the activeList. Currently all pcidevs are moved to activeList, but your change there moves from inactiveList to activeList, where AFAICT the inactiveList only has managed devices. See my conundrum now? The good news is the fridge is stocked with cold ones to help me ;-)
That is this change only grabs devices from the active list for the save; whereas, prior to this change it would pull from all pcidevs
So, I believe this part is correct albeit a bit confusing...
There are *no* devices that are in 'pcidevs' but *not* in the active list by this point - again, all objects in 'pcidevs' are used (or supposed to be used) for lookup purposes only.
I wonder if a better name would help making this easier to keep in mind even for someone who's looking at the code for the first time, or even myself two months from now :)
What about 'tmpDevices'? 'lookupDevices'?
pcidevs is fine to me... it's just a list of PCI hostdevs.
+ actual = virPCIDeviceListFindByIDs(mgr->activePCIHostdevs, + pcisrc->addr.domain, + pcisrc->addr.bus, + pcisrc->addr.slot, + pcisrc->addr.function);
/* Appropriate values for the unbind_from_stub, remove_slot * and reprobe properties of the device were set earlier * by virPCIDeviceDetach() */ - if (pci) { + if (actual) { VIR_DEBUG("Saving network configuration of PCI device %s", - virPCIDeviceGetName(pci)); + virPCIDeviceGetName(actual)); hostdev->origstates.states.pci.unbind_from_stub = - virPCIDeviceGetUnbindFromStub(pci); + virPCIDeviceGetUnbindFromStub(actual); hostdev->origstates.states.pci.remove_slot = - virPCIDeviceGetRemoveSlot(pci); + virPCIDeviceGetRemoveSlot(actual); hostdev->origstates.states.pci.reprobe = - virPCIDeviceGetReprobe(pci); + virPCIDeviceGetReprobe(actual); } }
@@ -844,17 +847,17 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr mgr, More documentation thoughts... Because what exists now for the function doesn't really make sense - it's only describing one variable.
Anyway, the purpose of this function is to take the device(s) passed on the hostdev list and return them to the host (?if they are managed?). This function is called either from hotplug device remove, hotplug device attach failure path, or from qemuProcessStop during process shutdown. When called from the host device remove or attach failure paths, there is only 1 hostdev in the list.
More documentation is definitely always a good thing, and this pretty much sums up what the function does :)
Prior to patch 15, the pcidev's was a copy of the "active list", now it's a list of all pcidevs found. Hopefully on either one of the two lists. That is I hope a found hostdev couldn't be on neither list.
See above: it's not a list of all PCI devices, just a list of PCI devices we're going to detach from the guest (and possibly reattach to the host) at this particular time.
I've flushed patch 15 from local cache ;-)
Prior to patch 15, step1 would either remove the device from pcidevs or from the activeList depending on the matching driver name. When done, the pcidevs would contain the list of devices just removed from the activeList. Note that virPCIDeviceListDel will repeat the active list virPCIDeviceListFind (essentially)...
After patch 15, the pcidevs gets reduced for devices not on the activeList or without a matching drv/dom name. Whether or not that is on the inactiveList isn't checked, but assumed.
Step 2 is I believe the antecedent of step 4 during the Prepare path. Step 4 would be called after Reset and before placing devices on active list replacing the net config for any PCINet device...
Prior to patch 15, step2 would walk the hostdev's list looking for devices on the pcidevs list (eg. the ones removed from the activeList). It would then take any PCINet device and restore it's config.
After patch 15 and with these changes, walk the inactiveList and perform the same call. This is not the devices we just removed from the activeList, so I don't think using the inactiveList in this case is right.
Yeah, this is totally bogus.
Looking up the device in the inactive list is the right thing to do *only after* patch 22 has been applied and step 2 is "Move devices from the active list to the inactive list".
With that in place we are guaranteed that the actual device will be in the inactive list, and the code above will be correct.
I believe I actually wrote this code after patch 22 and then squashed it in patch 20 on a misguided attempt to present changes in a more linear fashion :(
Yeah that makes perfect sense!
Before patch 15, step 3 would take any of the devices we removed from the activeList and perform a reset on them.
After patch 15, step 3 would do a similar function assuming that no device could not be on neither list.
NO device could NOT be on NEITHER list... My head is spinning :)
Actually, when we get here the devices are *guaranteed* to be in neither list, because we removed them from the active list in step 1. But that was the case even before patch 15, so it should not be a problem.
Hopefully this all makes sense - I keep reading it and going back and fort between old and new code. The comment left in the code after step 1 is most helpful...
I know how you feel! Thanks for sticking with it, hopefully the comments I've provided so far (especially the ones at the top of this message) are helpful in understanding the thought process behind the changes.
I'll get back to you with more comments tomorrow - bet you can't wait for them! ;)
No good deed goes unpunished ;-): As I read the current PreparePCIDevices code: Step1: Walk pcidevs, make some VFIO/ACS check... Make some PCI Node Device check... assume everything works as documented ;-) Step2: Walk pcidevs, if managed, call virPCIDeviceDetach to place *copy* of pci onto the inactive list, else VIR_DEBUG message Step3: Walk pcidevs, call virPCIDeviceReset on all devices (both those managed in inactiveList and those that are not managed) Step4: Walk hostdevs, SRIOV specific to replace network config (close my eyes, pray this works) Step5: Walk pcidevs, *adding* each device to activeList,. Since not copy, then device is on activeList *and* pcidevs at the same time, right? Step9 will "steal" from pcidevs to ensure virObjectUnref of pcidevs doesn't have virPCIDeviceListDispose free what's in activeList. (This is a key point...) Step6: Walk pcidevs, remove pci from inactiveList (free all memory of the copy initially placed on inactiveList). Step7: Walk pcidevs, if device found on activeList, call SetUsedBy Step8: Walk hostdevs, if device on pcidevs, then set unbind_from_stub, remove_slot, and reprobe (since everything on pcidevs is also in the activeList - magically the activeList data gets updated). Step9: Walk pcidevs, Stealing each element off of pcidevs (but doesn't virPCIDeviceFree the element so that virObjectUnref(pcidevs) won't find anything and do the free - because that element is on the activeList!). My notes (or what keeps bouncing around while working through this): 1. During the Prepare step, activeList add is not a copy, it uses VIR_APPEND_ELEMENT. IIUC, this means if 'dev' was dereferenced in virPCIDeviceListAdd after the APPEND, then there'd be a problem since dev would be set to NULL after the macro; however, since 'dev' is passed by value, that means the caller (e.g. the Prepare code) still can reference the element - it's just that the virPCIDevicePtr is listed in *two places*. You'll note other places in the code will Steal from one list and place on another, but this code doesn't do that until step9 where it steals all the pcidevs entries into apparently vaporware. "Theoretically" we really shouldn't reference activeList elements in pcidevs, but if we do reference them, then "magically" (so to speak) the activeList element would be updated. Personally it may be easier to use a *copy* for the activeList too and then let virObjectUnref handle the pcidevs entry deletion. 2. During Prepare, inactiveList add is done by copy (although one could argue that virPCIDeviceListAddCopy could be replaced by VIR_APPEND_ELEMENT_COPY I suppose). So now there are *two* copies of the same device - this is good and again, I think the model the activeList should use. 3. Current step9 just "steals" (or VIR_DELETE_ELEMENT) the element from pcidevs. This is "fine" since it's already/also on the activeList. You'll note other users of the macro would unref/free whatever memory was contained in the structure prior to removing from the list. Good thing we don't do that here ;-). If we don't steal the entry, then virObjectUnref(pcidevs) will call virPCIDeviceListDispose for us - your patch22 change to remove this may not be right... So given the current state of things, here's my view on the changes assuming my VIR_APPEND_ELEMENT viewpoint: Change #1: (what';s in the commit message) Alters step8 to use activeList instead of pcidevs. This part looks correct (perhaps worthy of it's own ACKable patch). Change #2: (not in the commit message) You jump into virHostdevReAttachPCIDevices and I then have to focus on a different algorithm and step process. Here's where I definitely believe you had patch22 in mind. But focusing on the code as it exists now... This code walks the hostdev's and will find anything on the pcidevs list which was on the former activeList and ensures the NetConfigRestore is run (e.g. the corollary of step4 in Prepare). Your change to ensure the device is on the inactiveList doesn't seem right at this point in time of the code. When you insert step2 in patch22, it would perhaps be more correct... Without peeking ahead too much, I think the insert unmanaged device onto inactiveList done in virHostdevReattachPCIDevice is incorrect at this point. Even if not incorrect, it's confusing. Thinking in terms of current code - step4 will take the everything that was formerly on the active list and: 1. Steal the pcidevs entry 2. Call virHostdevReattachPCIDevice with the stolen lookaside pcidevs entry... While it appears to be an actual, it isn't. Of course, I think you address that in patch 22, but trying not to get too far ahead. 2a. If the device is not managed, then add to the inactiveList and return (call virPCIDeviceFree if unsuccessfully add). What really confuses me here is why we place onto the inactiveList if not managed; however, you address that in patch22. Still it could be addressed earlier if that is a separate bug... 2b. If the device is managed, call virPCIDeviceReattach 2b1. Fail if on activeList (miserable) 2b2. Unbind stub 2b3. Call virPCIDeviceListDel -> Steal from inactiveList (search inactiveList for matching device by domain address, bus slot of the passed lookside pci device) -> call virPCIDeviceFree on the inactiveList entry 2c. Call virPCIDeviceFree on the lookaside list entry. 3. virObjectUnref(pcidevs) -> If I read the code right - each entry was stolen and properly removed so the virPCIDeviceListDispose has nothing to do. So it seems at this point, I would surmise virPCIDeviceReattach expects the pcidevs lookaside entry. The problem being insertion onto the inactiveList of an unmanaged device (something we avoided in Prepare). And yes, I'm still curious about virHostdevPCINodeDeviceDetach and how that plays into things. John
On Tue, 2016-03-15 at 14:33 -0400, John Ferlan wrote:
> On 03/14/2016 02:00 PM, Andrea Bolognani wrote:
> >
> > Word of warning, since this is very likely to get extremely
> > verbose: I fully expect to slip, here and there, and refer not
> > to the *current* situation as of this patch, but to the
> > *desired* situation at the end of the series.
>
> I've rebased to top of tree after your recent pushes... Seems like a
> good place to reset focus! I think I need a virtual whiteboard.
So, I've already replied to your comments about patch 22 yesterday
and patch 21 earlier today.
I believe a nice chunk of your concerns have been addressed by my
comments in the latter, so I'll mostly go quickly over them. Of
course let me know if I've glossed over something that was
actually a separate concern.
After I'm done with this reply I'll post a shortened series that
includes the commits that still haven't been pushed, reordering
and fixing them as needed to avoid the problems you noticed in
patches 20 and 21.
(I might decide to call it a day and go home between these two
events, it's getting kind of late already :)
> > Such *desired* situation is (not explicitly enough?) laid out
> > in patch 19, but I'll expand on it here for clarity:
> >
> > * the active list and inactive list contain the actual devices
>
> I understand/agree with the goal for actual devices in each list. One
> mental block is usage of virPCIDeviceListAdd for activeList and
> virPCIDeviceListAddCopy for inactiveList.
This mental block is one of the things patch 22 fixes :)
> The second mental block is
> whether these are for only managed devices (more on that later).
This should have been clarified by my comments to patch 21.
> > * each device is contained either in the active list, or in
> > the inactive list. It can't be in both
>
> Sure that's where you're headed, but with the current code this isn't
> necessarily true as a device could be on the inactiveList and activeList
> during patch 5, but that's the goal of patch 22.
Not sure about the reference to patch 5, but yes, this is the
*desired* situation, ie. how the code is supposed to behave after
the whole series has been applied. If we were already in the
desired situation, then we would need no patches ;)
> > * virPCIDevice instances in 'pcidevs' are only used for
> > looking up actual devices in the bookkeeping lists, contain
> > no valuable data and are disposable at any time
>
> This is where things start getting a bit fuzzy. I agree with the
> concept; however, the current implementation has a gotcha. Consider how
> VIR_APPEND_ELEMENT works. Initially 'pcidevs' is created as a new List,
> then for each 'hostdev' device, a new virPCIDevicePtr is created and
> added to the pcidevs list using virPCIDeviceListAdd. It's taken a bit to
> have VIR_APPEND_ELEMENT processing sink in for me ;-), but as I read the
> Prepare code it makes use of this processing model instead of creating a
> separate copy for the activeList.
So just to be clear, this is again about using virPCIDeviceListAdd()
when adding devices to the active list and virPCIDeviceListAddCopy()
when adding them to the inactive list, right? So one of the very
issues patch 22 is supposed to clear up :)
> > > First off - perhaps something for the previous documentation patch -
> > > PrepareDevices is called during the qemuProcessLaunch processing (eg
> > > domain startup) for all known host devices. It's also called during
> > > qemuDomainAttachHostPCIDevice or libxlDomainAttachHostPCIDevice
> > > (hotplug) for the *one* new device to be attached.
> > >
> > > The purpose of the function is to take the passed hostdev list, move the
> > > devices to a managed active host device list for the guest. Devices that
> > > do not have the managed attribute are not processed.
> > That's not true, though: unmanaged devices *are* processed, even
> > if less operations are performed on them. The part about
> > detaching them from the host is skipped, because it's supposed
> > to have been performed by the user alread, but that's it;
> > everything else (resetting them, moving them to the appropriate
> > bookkeeping list, storing information about what driver and
> > domain is using them) is just the same as managed devices.
> >
> I think this is where the confusion for me lies. I seem to have also
> mistyped a bit - perhaps it's the blurred lines between current and
> future perfect (hah!) state.
>
> Initially an unmanaged device is not placed on the inactiveList (e.g.
> step2 in the PreparePCIDevices code), but it is placed on the activeList
> (step5).
>
> Later on, we can place an unmanaged device on the inactiveList either in
> virHostdevReattachPCIDevice using the "stolen" 'pcidevs' entry during
> virHostdevReAttachPCIDevices or virHostdevPCINodeDeviceDetach where
> virPCIDeviceDetach uses a copy of the device. Although patch22 removes
> the addition into the inactiveList if !managed (new step5 and deletion
> of lines in ReattachPCIDevice). Still leaves the NodeDeviceDetach path
> to understand from whence that request came.
This should have been addressed by the comments on patch 21.
> > > > > /* Step 8: Now set the original states for hostdev def */
> > > > > for (i = 0; i < nhostdevs; i++) {
> > > > > - virPCIDevicePtr pci;
> > > > > + virPCIDevicePtr actual;
> > > > > virDomainHostdevDefPtr hostdev = hostdevs[i];
> > > > > virDomainHostdevSubsysPCIPtr pcisrc = &hostdev->source.subsys.u.pci;
> > > > >
> > > > > @@ -659,24 +659,27 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr,
> > > > > if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI)
> > > > > continue;
> > > > >
> > > > > - pci = virPCIDeviceListFindByIDs(pcidevs,
> > > > > - pcisrc->addr.domain,
> > > > > - pcisrc->addr.bus,
> > > > > - pcisrc->addr.slot,
> > > > > - pcisrc->addr.function);
> > > > > + /* We need to look up the actual device because it's the one
> > > > > + * that contains the information we care about (unbind_from_stub,
> > > > > + * remove_slot, reprobe) */
> > > > When a device goes from the inactivePCIHostdevs list to the
> > > > activePCIHostdevs list "at some point in time" in the future - does it
> > > > do a similar save?
> > >
> > > I'll answer my own question - because this is the Prepare function and
> > > we don't have to worry about it since everything is on the active list.
> > Exactly, all devices that go from the inactive list to the active
> > list do so via this function... We don't need to handle this
> > anywhere else. And by the time this specific lookup is performed,
> > all devices have already been moved to the active list (step 5).
> >
> Right, but if I read patch22 correctly (jumping slightly ahead), only
> managed devices would be placed onto the activeList. Currently all
> pcidevs are moved to activeList, but your change there moves from
> inactiveList to activeList, where AFAICT the inactiveList only has
> managed devices.
>
> See my conundrum now? The good news is the fridge is stocked with cold
> ones to help me ;-)
Again, comments on patch 21. And for some reason I kinda feel
thirsty now, weird ;)
> > > Before patch 15, step 3 would take any of the devices we removed from
> > > the activeList and perform a reset on them.
> > >
> > > After patch 15, step 3 would do a similar function assuming that no
> > > device could not be on neither list.
> > NO device could NOT be on NEITHER list... My head is spinning :)
> >
> > Actually, when we get here the devices are *guaranteed* to be in
> > neither list, because we removed them from the active list in
> > step 1. But that was the case even before patch 15, so it should
> > not be a problem.
> >
> > >
> > > Hopefully this all makes sense - I keep reading it and going back and
> > > fort between old and new code. The comment left in the code after step
> > > 1 is most helpful...
> > I know how you feel! Thanks for sticking with it, hopefully the
> > comments I've provided so far (especially the ones at the top of
> > this message) are helpful in understanding the thought process
> > behind the changes.
> >
> > I'll get back to you with more comments tomorrow - bet you can't
> > wait for them! ;)
> No good deed goes unpunished ;-):
>
>
> As I read the current PreparePCIDevices code:
>
> Step1: Walk pcidevs, make some VFIO/ACS check... Make some PCI Node
> Device check... assume everything works as documented ;-)
>
> Step2: Walk pcidevs, if managed, call virPCIDeviceDetach to place *copy*
> of pci onto the inactive list, else VIR_DEBUG message
>
> Step3: Walk pcidevs, call virPCIDeviceReset on all devices (both those
> managed in inactiveList and those that are not managed)
>
> Step4: Walk hostdevs, SRIOV specific to replace network config (close my
> eyes, pray this works)
>
> Step5: Walk pcidevs, *adding* each device to activeList,. Since not
> copy, then device is on activeList *and* pcidevs at the same time,
> right? Step9 will "steal" from pcidevs to ensure virObjectUnref of
> pcidevs doesn't have virPCIDeviceListDispose free what's in activeList.
> (This is a key point...)
>
> Step6: Walk pcidevs, remove pci from inactiveList (free all memory of
> the copy initially placed on inactiveList).
>
> Step7: Walk pcidevs, if device found on activeList, call SetUsedBy
>
> Step8: Walk hostdevs, if device on pcidevs, then set unbind_from_stub,
> remove_slot, and reprobe (since everything on pcidevs is also in the
> activeList - magically the activeList data gets updated).
>
> Step9: Walk pcidevs, Stealing each element off of pcidevs (but doesn't
> virPCIDeviceFree the element so that virObjectUnref(pcidevs) won't find
> anything and do the free - because that element is on the activeList!).
>
> My notes (or what keeps bouncing around while working through this):
>
> 1. During the Prepare step, activeList add is not a copy, it uses
> VIR_APPEND_ELEMENT. IIUC, this means if 'dev' was dereferenced in
> virPCIDeviceListAdd after the APPEND, then there'd be a problem since
> dev would be set to NULL after the macro; however, since 'dev' is passed
> by value, that means the caller (e.g. the Prepare code) still can
> reference the element - it's just that the virPCIDevicePtr is listed in
> *two places*.
Exactly, and doing so causes nothing but grief - which is why
patch 22 gets rid of this behaviour completely.
> You'll note other places in the code will Steal from one
> list and place on another, but this code doesn't do that until step9
> where it steals all the pcidevs entries into apparently vaporware.
> "Theoretically" we really shouldn't reference activeList elements in
> pcidevs, but if we do reference them, then "magically" (so to speak) the
> activeList element would be updated. Personally it may be easier to use
> a *copy* for the activeList too and then let virObjectUnref handle the
> pcidevs entry deletion.
Claiming that I completely agree is an understatement!
> 2. During Prepare, inactiveList add is done by copy (although one could
> argue that virPCIDeviceListAddCopy could be replaced by
> VIR_APPEND_ELEMENT_COPY I suppose). So now there are *two* copies of the
> same device - this is good and again, I think the model the activeList
> should use.
Yes!
> 3. Current step9 just "steals" (or VIR_DELETE_ELEMENT) the element from
> pcidevs. This is "fine" since it's already/also on the activeList.
> You'll note other users of the macro would unref/free whatever memory
> was contained in the structure prior to removing from the list. Good
> thing we don't do that here ;-).
As noted previously, the current code is correct, just *extremely*
hard to get right! So kudos to whoever managed such a feat :)
> If we don't steal the entry, then
> virObjectUnref(pcidevs) will call virPCIDeviceListDispose for us - your
> patch22 change to remove this may not be right...
I believe I've already explained this in detail in my reply to your
comments on patch 22, so I won't repeat myself here. If you still
think this is unclear, just let me know.
> So given the current state of things, here's my view on the changes
> assuming my VIR_APPEND_ELEMENT viewpoint:
>
> Change #1: (what';s in the commit message)
> Alters step8 to use activeList instead of pcidevs. This part looks
> correct (perhaps worthy of it's own ACKable patch).
>
> Change #2: (not in the commit message)
> You jump into virHostdevReAttachPCIDevices and I then have to focus on a
> different algorithm and step process. Here's where I definitely believe
> you had patch22 in mind.
Yeah, I've already confirmed that was the case :)
> But focusing on the code as it exists now... This code walks the
> hostdev's and will find anything on the pcidevs list which was on the
> former activeList and ensures the NetConfigRestore is run (e.g. the
> corollary of step4 in Prepare).
>
> Your change to ensure the device is on the inactiveList doesn't seem
> right at this point in time of the code. When you insert step2 in
> patch22, it would perhaps be more correct...
Yup.
> Without peeking ahead too much, I think the insert unmanaged device onto
> inactiveList done in virHostdevReattachPCIDevice is incorrect at this
> point. Even if not incorrect, it's confusing.
No, you're right, it's actually incorrect.
> Thinking in terms of current code - step4 will take the everything that
> was formerly on the active list and:
>
> 1. Steal the pcidevs entry
>
> 2. Call virHostdevReattachPCIDevice with the stolen lookaside pcidevs
> entry... While it appears to be an actual, it isn't. Of course, I think
> you address that in patch 22, but trying not to get too far ahead.
>
> 2a. If the device is not managed, then add to the inactiveList and
> return (call virPCIDeviceFree if unsuccessfully add). What really
> confuses me here is why we place onto the inactiveList if not managed;
> however, you address that in patch22. Still it could be addressed
> earlier if that is a separate bug...
I would have to go back to patch 22 to make sure this is really
handled correctly there: I'm pretty confident it is, but I've
been wrong before! Most recently, while writing this series :(
The incorrect behaviour you've noticed before has actually been
introduced by patch 15. Before that, 'pcidevs' in
virHostdevReAttachPCIDevices() was obtained by copying instances
off the active list, hence virHostdevReattachPCIDevice() would
really be passed (a copy of) an actual; now that's no longer the
case, and the code has become incorrect.
Will fix as part of the respin.
> 2b. If the device is managed, call virPCIDeviceReattach
>
> 2b1. Fail if on activeList (miserable)
> 2b2. Unbind stub
> 2b3. Call virPCIDeviceListDel
> -> Steal from inactiveList (search inactiveList for matching
> device by domain address, bus slot of the passed lookside pci device)
> -> call virPCIDeviceFree on the inactiveList entry
>
> 2c. Call virPCIDeviceFree on the lookaside list entry.
>
> 3. virObjectUnref(pcidevs)
>
> -> If I read the code right - each entry was stolen and properly
> removed so the virPCIDeviceListDispose has nothing to do.
>
> So it seems at this point, I would surmise virPCIDeviceReattach expects
> the pcidevs lookaside entry.
Nope, definitely wants the actual device.
> The problem being insertion onto the
> inactiveList of an unmanaged device (something we avoided in Prepare).
Well, unmanaged devices are supposed to end up in the inactive
list at the end of this function... They will be removed from
there by virHostdevPCINodeDeviceReAttach().
> And yes, I'm still curious about virHostdevPCINodeDeviceDetach and how
> that plays into things.
What exactly is confusing you about that function?
So well, it's almost dinner time here, respin's coming tomorrow
I guess :)
Cheers.
--
Andrea Bolognani
Software Engineer - Virtualization Team
[...]
And yes, I'm still curious about virHostdevPCINodeDeviceDetach and how that plays into things.
What exactly is confusing you about that function?
It's somewhere that puts devices on the inactiveList via virPCIDeviceDetach and my eyes/brain kept telling me, don't go there, you don't want to confuse yourself, stay away, it's a trap. I think it was more of how does it play in the whole scheme of things... Then something written in 21 or 22 perhaps made me think that part of the process of "self managing" whether the device is attached to the host or not is something telling libvirt via that function that the device was detached. It doesn't care about the domain, just the device.
So well, it's almost dinner time here, respin's coming tomorrow I guess :)
I'll go restock the fridge! John
And yes, I'm still curious about virHostdevPCINodeDeviceDetach and how that plays into things. What exactly is confusing you about that function? It's somewhere that puts devices on the inactiveList via virPCIDeviceDetach and my eyes/brain kept telling me, don't go there, you don't want to confuse yourself, stay away, it's a trap. I think it was more of how does it
Then something written in 21 or 22 perhaps made me think that part of the
On Wed, 2016-03-16 at 15:05 -0400, John Ferlan wrote: play in the whole scheme of things... process of "self managing" whether the device is attached to the
host or not is something telling libvirt via that function that the device was detached. It doesn't care about the domain, just the device.
Yeah, basically that function is used when the user is willing to take on part of the responsibility himself. Or needs to. If you recall the hostdev series I posted before this one, it was all about making sure that "detach from host" and "reattach to host" do exactly the same thing regardless of whether they're performed explicitly by the user or implicitly as part of working with managed devices. That's the end goal[1]. Cheers. [1] Well, the end goal is really fixing #1372300, but we can't do that in a sane way before we've gotten rid of this code duplication :) -- Andrea Bolognani Software Engineer - Virtualization Team
On Wed, 2016-03-16 at 19:50 +0100, Andrea Bolognani wrote:
Thinking in terms of current code - step4 will take the everything that was formerly on the active list and: 1. Steal the pcidevs entry 2. Call virHostdevReattachPCIDevice with the stolen lookaside pcidevs entry... While it appears to be an actual, it isn't. Of course, I think you address that in patch 22, but trying not to get too far ahead. 2a. If the device is not managed, then add to the inactiveList and return (call virPCIDeviceFree if unsuccessfully add). What really confuses me here is why we place onto the inactiveList if not managed; however, you address that in patch22. Still it could be addressed earlier if that is a separate bug... I would have to go back to patch 22 to make sure this is really handled correctly there: I'm pretty confident it is, but I've been wrong before! Most recently, while writing this series :( The incorrect behaviour you've noticed before has actually been introduced by patch 15. Before that, 'pcidevs' in virHostdevReAttachPCIDevices() was obtained by copying instances off the active list, hence virHostdevReattachPCIDevice() would really be passed (a copy of) an actual; now that's no longer the case, and the code has become incorrect. Will fix as part of the respin.
Turns out that fixing this in a separate patch would basically mean reverting the patch that introduced the behaviour in the first place. I don't think it would be worth the effort. I've confirmed that the fix is actually in patch 22, as suspected, so we should be good anyway. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
Unmanaged devices, as the name suggests, are not detached automatically from the host by libvirt before being attached to a guest: it's the user's responsability to detach them manually beforehand. If that preliminary step has not been performed, the attach operation can't complete successfully. Instead of relying on the lower layers to error out with cryptic messages such as error: Failed to attach device from /tmp/hostdev.xml error: Path '/dev/vfio/12' is not accessible: No such file or directory prevent the situation altogether and provide the user with a more useful error message. --- src/util/virhostdev.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index 03c3445..d1529c5 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -576,6 +576,13 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, mgr->inactivePCIHostdevs) < 0) goto reattachdevs; } else { + if (!virPCIDeviceListFind(mgr->inactivePCIHostdevs, pci)) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("Unmanaged PCI device %s must be manually " + "detached from the host"), + virPCIDeviceGetName(pci)); + goto reattachdevs; + } VIR_DEBUG("Not detaching unmanaged PCI device %s", virPCIDeviceGetName(pci)); } -- 2.5.0
On 03/07/2016 12:24 PM, Andrea Bolognani wrote:
Unmanaged devices, as the name suggests, are not detached automatically from the host by libvirt before being attached to a guest: it's the user's responsability to detach them manually beforehand. If that preliminary step has not been performed, the attach operation can't complete successfully.
Instead of relying on the lower layers to error out with cryptic messages such as
error: Failed to attach device from /tmp/hostdev.xml error: Path '/dev/vfio/12' is not accessible: No such file or directory
prevent the situation altogether and provide the user with a more useful error message. --- src/util/virhostdev.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index 03c3445..d1529c5 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -576,6 +576,13 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, mgr->inactivePCIHostdevs) < 0)
Is this in the right place? Consider a few lines above: /* Step 1: validate that non-managed device isn't in use, ... Second question - how would the device get on the inactiveList initially? Looking at virPCIDeviceListAdd calls for inactiveList. 'pcidevs' is the list of all devices both managed and unmanaged 'activeList' is populated in step 5 and virHostdevUpdateActivePCIDevices 'inactiveList' is populated in 'inactivedevs:' label, in step 2 of virHostdevReAttachPCIDevices, and in virPCIDeviceDetach via virPCIDeviceListAddCopy *if* the device is managed in step 2 of Prepare. I don't disagree this is an important step, but it's the "how" we determine this that I'm questioning. John
goto reattachdevs; } else { + if (!virPCIDeviceListFind(mgr->inactivePCIHostdevs, pci)) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("Unmanaged PCI device %s must be manually " + "detached from the host"), + virPCIDeviceGetName(pci)); + goto reattachdevs; + } VIR_DEBUG("Not detaching unmanaged PCI device %s", virPCIDeviceGetName(pci)); }
On Sat, 2016-03-12 at 09:23 -0500, John Ferlan wrote:
On 03/07/2016 12:24 PM, Andrea Bolognani wrote:
Unmanaged devices, as the name suggests, are not detached automatically from the host by libvirt before being attached to a guest: it's the user's responsability to detach them manually beforehand. If that preliminary step has not been performed, the attach operation can't complete successfully. Instead of relying on the lower layers to error out with cryptic messages such as error: Failed to attach device from /tmp/hostdev.xml error: Path '/dev/vfio/12' is not accessible: No such file or directory prevent the situation altogether and provide the user with a more useful error message. --- src/util/virhostdev.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index 03c3445..d1529c5 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -576,6 +576,13 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, mgr->inactivePCIHostdevs) < 0) Is this in the right place? Consider a few lines above: /* Step 1: validate that non-managed device isn't in use, ...
Well, that comment is not really 100% accurate now that I look at it. What the code in step 1 is really doing is making sure that the device is assignable (virPCIDeviceIsAssignable(), haven't really investigated that function TBH so I'm just assuming it's doing something valuable ;) and that none of the devices we're about to assign to the guest have already been assigned to another guest - or, in the case of VFIO, that no devices that are in the same IOMMU group as a device we're about to assign to the guest have already been assigned to another guest. There are actually no checks on unmanaged devices, and rightly so: it's totally okay to add unmanaged devices to a guest! We just have to make sure the user has actually taken care to detach the device from the host first, hence the new check I'm adding with this commit. The comment for step 1 needs some work, though, as you point out...
Second question - how would the device get on the inactiveList initially? Looking at virPCIDeviceListAdd calls for inactiveList. 'pcidevs' is the list of all devices both managed and unmanaged 'activeList' is populated in step 5 and virHostdevUpdateActivePCIDevices 'inactiveList' is populated in 'inactivedevs:' label, in step 2 of virHostdevReAttachPCIDevices, and in virPCIDeviceDetach via virPCIDeviceListAddCopy *if* the device is managed in step 2 of Prepare.
'pcidevs' is not the list of all devices, it's the list of (really just names of) devices that we're handling at the moment. It's our working set, basically. But yes, devices in 'pcidevs' can be both managed or unmanaged. As for your question, a device might end up in the inactive list because of: * PreparePCIDevices(), step 2, if managed. Just temporarily, it's moved to the active list in step 5 * ReattachPCIDevices(), step 4, whether managed or unmanaged. If it's managed is also removed from the inactive list in the same step, if it's unmanaged it will remain in the inactive list * PCINodeDeviceDetach() This is not including rollback paths. So it ends up in the inactive list when it's either *not yet* active, or *no longer* active.
I don't disagree this is an important step, but it's the "how" we determine this that I'm questioning.
Hopefully the above helped :) Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
On 03/15/2016 07:54 AM, Andrea Bolognani wrote:
On Sat, 2016-03-12 at 09:23 -0500, John Ferlan wrote:
On 03/07/2016 12:24 PM, Andrea Bolognani wrote:
Unmanaged devices, as the name suggests, are not detached automatically from the host by libvirt before being attached to a guest: it's the user's responsability to detach them manually beforehand. If that preliminary step has not been performed, the attach operation can't complete successfully.
Instead of relying on the lower layers to error out with cryptic messages such as
error: Failed to attach device from /tmp/hostdev.xml error: Path '/dev/vfio/12' is not accessible: No such file or directory
prevent the situation altogether and provide the user with a more useful error message. --- src/util/virhostdev.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index 03c3445..d1529c5 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -576,6 +576,13 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, mgr->inactivePCIHostdevs) < 0)
Is this in the right place?
Consider a few lines above:
/* Step 1: validate that non-managed device isn't in use, ...
Well, that comment is not really 100% accurate now that I look at it.
What the code in step 1 is really doing is making sure that the device is assignable (virPCIDeviceIsAssignable(), haven't really investigated that function TBH so I'm just assuming it's doing something valuable ;) and that none of the devices we're about to assign to the guest have already been assigned to another guest - or, in the case of VFIO, that no devices that are in the same IOMMU group as a device we're about to assign to the guest have already been assigned to another guest.
There are actually no checks on unmanaged devices, and rightly so: it's totally okay to add unmanaged devices to a guest!
We just have to make sure the user has actually taken care to detach the device from the host first, hence the new check I'm adding with this commit.
The comment for step 1 needs some work, though, as you point out...
Second question - how would the device get on the inactiveList initially? Looking at virPCIDeviceListAdd calls for inactiveList.
'pcidevs' is the list of all devices both managed and unmanaged
'activeList' is populated in step 5 and virHostdevUpdateActivePCIDevices
'inactiveList' is populated in 'inactivedevs:' label, in step 2 of virHostdevReAttachPCIDevices, and in virPCIDeviceDetach via virPCIDeviceListAddCopy *if* the device is managed in step 2 of Prepare.
'pcidevs' is not the list of all devices, it's the list of (really just names of) devices that we're handling at the moment. It's our working set, basically. But yes, devices in 'pcidevs' can be both managed or unmanaged.
As for your question, a device might end up in the inactive list because of:
* PreparePCIDevices(), step 2, if managed. Just temporarily, it's moved to the active list in step 5
* ReattachPCIDevices(), step 4, whether managed or unmanaged. If it's managed is also removed from the inactive list in the same step, if it's unmanaged it will remain in the inactive list
* PCINodeDeviceDetach()
This is not including rollback paths. So it ends up in the inactive list when it's either *not yet* active, or *no longer* active.
I don't disagree this is an important step, but it's the "how" we determine this that I'm questioning.
Hopefully the above helped :)
Partially, let's consider an initial startup where Prepare is called (and I assume we don't call either virHostdevReattachPCIDevice or virHostdevPCINodeDeviceDetach yet). So how does this code check if the device has been manually detached from the host? Step 1 doesn't add to the inactiveList and step 2 only adds managed devices. How would that lookaside element be on the inactiveList - what put it there? Is there some magic w/ virHostdevPCINodeDeviceDetach that I'm missing? John
On Tue, 2016-03-15 at 14:38 -0400, John Ferlan wrote:
As for your question, a device might end up in the inactive list because of: * PreparePCIDevices(), step 2, if managed. Just temporarily, it's moved to the active list in step 5 * ReattachPCIDevices(), step 4, whether managed or unmanaged. If it's managed is also removed from the inactive list in the same step, if it's unmanaged it will remain in the inactive list * PCINodeDeviceDetach() This is not including rollback paths. So it ends up in the inactive list when it's either *not yet* active, or *no longer* active.
I don't disagree this is an important step, but it's the "how" we determine this that I'm questioning. Hopefully the above helped :) Partially, let's consider an initial startup where Prepare is called (and I assume we don't call either virHostdevReattachPCIDevice or virHostdevPCINodeDeviceDetach yet). So how does this code check if the device has been manually detached from the host? Step 1 doesn't add to the inactiveList and step 2 only adds managed devices. How would that lookaside element be on the inactiveList - what put it there? Is there some magic w/ virHostdevPCINodeDeviceDetach that I'm missing?
Oh, I see what you mean now! The long and short of it is that the new code simply doesn't handle the situation you're describing :) I wonder what's the proper way to handle this. The way it worked before was *way* implicit, and the less implicit stuff we have, the better. I guess checking the driver the device is bound to, and adding it to the inactive list if it's one of the known stub drivers would do the trick for now. Does that sound reasonable? I'll try and cobble together an implementation. Long term, as I mentioned in the past, we should implement a way for the active and inactive list to be stored to disk and read back at startup time, ensuring all information are preserved across daemon restarts. I'd like to work on that as a follow-up enhancement, though. For now, creating a solid base to work on (while avoiding regressions) should be the main focus. Thanks for catching this! -- Andrea Bolognani Software Engineer - Virtualization Team
After this patch, ownership of virPCIDevice instances is very easy to keep track of: for each host PCI device, the only instance that actually matters is the one inside one of the bookkeeping list. Whenever some operation needs to be performed on a PCI device, the actual device is looked up first; when this is not the case, a comment explains the reason. --- src/util/virhostdev.c | 118 +++++++++++++++++++++++++++----------------------- 1 file changed, 65 insertions(+), 53 deletions(-) diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index d1529c5..b2f54cd 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -618,28 +618,22 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, last_processed_hostdev_vf = i; } - /* Step 5: Now mark all the devices as active */ + /* Step 5: Move devices from the inactive list to the active list */ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i); + virPCIDevicePtr actual; - VIR_DEBUG("Adding PCI device %s to active list", + VIR_DEBUG("Removing PCI device %s from inactive list", virPCIDeviceGetName(pci)); - if (virPCIDeviceListAdd(mgr->activePCIHostdevs, pci) < 0) - goto inactivedevs; - } - - /* Step 6: Now remove the devices from inactive list. */ - for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { - virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i); + actual = virPCIDeviceListSteal(mgr->inactivePCIHostdevs, pci); - VIR_DEBUG("Removing PCI device %s from inactive list", + VIR_DEBUG("Adding PCI device %s to active list", virPCIDeviceGetName(pci)); - virPCIDeviceListDel(mgr->inactivePCIHostdevs, pci); + if (!actual || virPCIDeviceListAdd(mgr->activePCIHostdevs, actual) < 0) + goto inactivedevs; } - /* Step 7: Now set the used_by_domain of the device in - * activePCIHostdevs as domain name. - */ + /* Step 6: Set driver and domain information */ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr pci, actual; @@ -655,7 +649,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, virPCIDeviceSetUsedBy(actual, drv_name, dom_name); } - /* Step 8: Now set the original states for hostdev def */ + /* Step 7: Now set the original states for hostdev def */ for (i = 0; i < nhostdevs; i++) { virPCIDevicePtr actual; virDomainHostdevDefPtr hostdev = hostdevs[i]; @@ -690,23 +684,25 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, } } - /* Step 9: Now steal all the devices from pcidevs */ - while (virPCIDeviceListCount(pcidevs) > 0) - virPCIDeviceListStealIndex(pcidevs, 0); - ret = 0; goto cleanup; inactivedevs: - /* Only steal all the devices from activePCIHostdevs. We will - * free them in virObjectUnref(). - */ + /* Move devices back to the inactive list so that they can be + * processed properly below (reattachdevs label) */ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i); + virPCIDevicePtr actual; VIR_DEBUG("Removing PCI device %s from active list", virPCIDeviceGetName(pci)); - virPCIDeviceListSteal(mgr->activePCIHostdevs, pci); + if (!(actual = virPCIDeviceListSteal(mgr->activePCIHostdevs, pci))) + continue; + + VIR_DEBUG("Adding PCI device %s to inactive list", + virPCIDeviceGetName(pci)); + if (virPCIDeviceListAdd(mgr->inactivePCIHostdevs, actual) < 0) + continue; } resetvfnetconfig: @@ -716,16 +712,22 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, reattachdevs: for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i); + virPCIDevicePtr actual; - if (virPCIDeviceGetManaged(pci)) { + /* We need to look up the actual device because that's what + * virPCIDeviceReattach() exepects as its argument */ + if (!(actual = virPCIDeviceListFind(mgr->inactivePCIHostdevs, pci))) + continue; + + if (virPCIDeviceGetManaged(actual)) { VIR_DEBUG("Reattaching managed PCI device %s", - virPCIDeviceGetName(pci)); - ignore_value(virPCIDeviceReattach(pci, + virPCIDeviceGetName(actual)); + ignore_value(virPCIDeviceReattach(actual, mgr->activePCIHostdevs, mgr->inactivePCIHostdevs)); } else { VIR_DEBUG("Not reattaching unmanaged PCI device %s", - virPCIDeviceGetName(pci)); + virPCIDeviceGetName(actual)); } } @@ -745,17 +747,6 @@ static void virHostdevReattachPCIDevice(virHostdevManagerPtr mgr, virPCIDevicePtr actual) { - /* If the device is not managed and was attached to guest - * successfully, it must have been inactive. - */ - if (!virPCIDeviceGetManaged(actual)) { - VIR_DEBUG("Adding unmanaged PCI device %s to inactive list", - virPCIDeviceGetName(actual)); - if (virPCIDeviceListAdd(mgr->inactivePCIHostdevs, actual) < 0) - virPCIDeviceFree(actual); - return; - } - /* Wait for device cleanup if it is qemu/kvm */ if (virPCIDeviceGetStubDriver(actual) == VIR_PCI_STUB_DRIVER_KVM) { int retries = 100; @@ -774,7 +765,6 @@ virHostdevReattachPCIDevice(virHostdevManagerPtr mgr, err ? err->message : _("unknown error")); virResetError(err); } - virPCIDeviceFree(actual); } /* @oldStateDir: @@ -836,17 +826,29 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr mgr, continue; } + i++; + } + + /* Step 2: Move devices from the active list to the inactive list */ + for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { + virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i); + virPCIDevicePtr actual; + VIR_DEBUG("Removing PCI device %s from active list", virPCIDeviceGetName(pci)); - virPCIDeviceListDel(mgr->activePCIHostdevs, pci); - i++; + actual = virPCIDeviceListSteal(mgr->activePCIHostdevs, pci); + + if (actual) { + VIR_DEBUG("Adding PCI device %s to inactive list", + virPCIDeviceGetName(pci)); + virPCIDeviceListAdd(mgr->inactivePCIHostdevs, actual); + } } - /* At this point, any device that had been used by the guest is in - * pcidevs, but has been removed from activePCIHostdevs. - */ + /* At this point, any device that had been used by the guest has been + * moved to the inactive list */ - /* Step 2: restore original network config of hostdevs that used + /* Step 3: restore original network config of hostdevs that used * <interface type='hostdev'> */ for (i = 0; i < nhostdevs; i++) { @@ -871,7 +873,7 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr mgr, } } - /* Step 3: perform a PCI Reset on all devices */ + /* Step 4: perform a PCI Reset on all devices */ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i); @@ -888,16 +890,26 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr mgr, } } - /* Step 4: reattach devices to their host drivers (if managed) or place - * them on the inactive list (if not managed) - */ - while (virPCIDeviceListCount(pcidevs) > 0) { - virPCIDevicePtr pci = virPCIDeviceListStealIndex(pcidevs, 0); - virHostdevReattachPCIDevice(mgr, pci); + /* Step 5: Reattach managed devices to their host drivers; unmanaged + * devices don't need to be processed further */ + for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { + virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i); + virPCIDevicePtr actual; + + /* We need to look up the actual device because that's what + * virHostdevReattachPCIDevice() expects as its argument */ + if (!(actual = virPCIDeviceListFind(mgr->inactivePCIHostdevs, pci))) + continue; + + if (virPCIDeviceGetManaged(actual)) + virHostdevReattachPCIDevice(mgr, actual); + else + VIR_DEBUG("Not reattaching unmanaged PCI device %s", + virPCIDeviceGetName(actual)); } - virObjectUnref(pcidevs); cleanup: + virObjectUnref(pcidevs); virObjectUnlock(mgr->activePCIHostdevs); virObjectUnlock(mgr->inactivePCIHostdevs); } -- 2.5.0
On 03/07/2016 12:24 PM, Andrea Bolognani wrote:
After this patch, ownership of virPCIDevice instances is very easy to keep track of: for each host PCI device, the only instance that actually matters is the one inside one of the bookkeeping list.
Whenever some operation needs to be performed on a PCI device, the actual device is looked up first; when this is not the case, a comment explains the reason. --- src/util/virhostdev.c | 118 +++++++++++++++++++++++++++----------------------- 1 file changed, 65 insertions(+), 53 deletions(-)
FWIW: from your cover - splitting this up... I think it's possible that the changes for calling virHostdevReattachPCIDevice with an actual device could be in a separate patch... But I certainly see how this and that are "tied" together especially if you're considering the failure path through reattachdevs where you've definitely put things on the inactiveList. There may also be a 4 patches overall in here since I see the Reattach code could place an unmanaged device on the inactiveList (which you modify in this patch) and I think the ReAttach code adjustments could be separate too. I think you're on the right track, with all these changes - it's some minor details. I've been looking at the changes, walking away, looking at them, so hopefully the following is followable! BTW: I chose not to look at the last 2 patches as I think there are too many questions left over
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index d1529c5..b2f54cd 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -618,28 +618,22 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, last_processed_hostdev_vf = i; }
- /* Step 5: Now mark all the devices as active */ + /* Step 5: Move devices from the inactive list to the active list */ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i); + virPCIDevicePtr actual;
- VIR_DEBUG("Adding PCI device %s to active list", + VIR_DEBUG("Removing PCI device %s from inactive list", virPCIDeviceGetName(pci)); - if (virPCIDeviceListAdd(mgr->activePCIHostdevs, pci) < 0) - goto inactivedevs; - } - - /* Step 6: Now remove the devices from inactive list. */ - for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { - virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i); + actual = virPCIDeviceListSteal(mgr->inactivePCIHostdevs, pci);
- VIR_DEBUG("Removing PCI device %s from inactive list", + VIR_DEBUG("Adding PCI device %s to active list", virPCIDeviceGetName(pci)); - virPCIDeviceListDel(mgr->inactivePCIHostdevs, pci); + if (!actual || virPCIDeviceListAdd(mgr->activePCIHostdevs, actual) < 0) + goto inactivedevs; }
This is so much more logical to me than the previous code. One question though - why not use virPCIDeviceListStealIndex? Theoretically the inactiveList and the pcidevs list is the same right? So for every devices on pcidevs - steal index 0 of inactiveList and place it on activeList. It's just a slight optimization.
- /* Step 7: Now set the used_by_domain of the device in - * activePCIHostdevs as domain name. - */ + /* Step 6: Set driver and domain information */ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr pci, actual;
@@ -655,7 +649,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, virPCIDeviceSetUsedBy(actual, drv_name, dom_name); }
- /* Step 8: Now set the original states for hostdev def */ + /* Step 7: Now set the original states for hostdev def */ for (i = 0; i < nhostdevs; i++) { virPCIDevicePtr actual; virDomainHostdevDefPtr hostdev = hostdevs[i]; @@ -690,23 +684,25 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, } }
- /* Step 9: Now steal all the devices from pcidevs */ - while (virPCIDeviceListCount(pcidevs) > 0) - virPCIDeviceListStealIndex(pcidevs, 0); -
Help me out... Why were these lines removed? The inactiveDevs is initially a copy of whatever is on 'pcidevs'... Thus activeDevs takes from there and not pcidevs, right?
ret = 0; goto cleanup;
inactivedevs: - /* Only steal all the devices from activePCIHostdevs. We will - * free them in virObjectUnref(). - */ + /* Move devices back to the inactive list so that they can be + * processed properly below (reattachdevs label) */ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i); + virPCIDevicePtr actual;
VIR_DEBUG("Removing PCI device %s from active list", virPCIDeviceGetName(pci)); - virPCIDeviceListSteal(mgr->activePCIHostdevs, pci); + if (!(actual = virPCIDeviceListSteal(mgr->activePCIHostdevs, pci))) + continue; + + VIR_DEBUG("Adding PCI device %s to inactive list", + virPCIDeviceGetName(pci)); + if (virPCIDeviceListAdd(mgr->inactivePCIHostdevs, actual) < 0) + continue;
Another option is to VIR_WARN (INFO, DEBUG, whatever) the failure. You may also want to clear the error as well (virResetLastError). There are two possible errors - it already exists on the list and memory allocation errors. The continue is superfluous since this is the end of the loop - it only matters if someone adds more checks afterwards
}
resetvfnetconfig: @@ -716,16 +712,22 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, reattachdevs: for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i); + virPCIDevicePtr actual;
- if (virPCIDeviceGetManaged(pci)) { + /* We need to look up the actual device because that's what + * virPCIDeviceReattach() exepects as its argument */
"expects" As I read it virPCIDeviceReattach look for the device on the activeList, but will "blindly" remove from the inactiveList *if* provided. That's only true for xen_driver and virpcitest. I believe this process is the more correct one.
+ if (!(actual = virPCIDeviceListFind(mgr->inactivePCIHostdevs, pci))) + continue; + + if (virPCIDeviceGetManaged(actual)) { VIR_DEBUG("Reattaching managed PCI device %s", - virPCIDeviceGetName(pci)); - ignore_value(virPCIDeviceReattach(pci, + virPCIDeviceGetName(actual)); + ignore_value(virPCIDeviceReattach(actual, mgr->activePCIHostdevs, mgr->inactivePCIHostdevs)); } else { VIR_DEBUG("Not reattaching unmanaged PCI device %s", - virPCIDeviceGetName(pci)); + virPCIDeviceGetName(actual)); } }
@@ -745,17 +747,6 @@ static void virHostdevReattachPCIDevice(virHostdevManagerPtr mgr, virPCIDevicePtr actual) { - /* If the device is not managed and was attached to guest - * successfully, it must have been inactive. - */ - if (!virPCIDeviceGetManaged(actual)) { - VIR_DEBUG("Adding unmanaged PCI device %s to inactive list", - virPCIDeviceGetName(actual)); - if (virPCIDeviceListAdd(mgr->inactivePCIHostdevs, actual) < 0) - virPCIDeviceFree(actual); - return; - } -
Is it just me getting really confused - this code used to add devices to the inactiveList even though (at some point in time) the Prepare code would "drop" them? E.G. step 2 before any of these changes. Was this perhaps what caused you to add that PCIDeviceListFind code in step 2 of patch 21 that I questioned? Perhaps this alone deserves it's own patch and I think is it's own problem since the lookaside lists are for devices that are managed, right?
/* Wait for device cleanup if it is qemu/kvm */ if (virPCIDeviceGetStubDriver(actual) == VIR_PCI_STUB_DRIVER_KVM) { int retries = 100; @@ -774,7 +765,6 @@ virHostdevReattachPCIDevice(virHostdevManagerPtr mgr, err ? err->message : _("unknown error")); virResetError(err); } - virPCIDeviceFree(actual);
Hmm... mental note - what changes here... to cause us not to need to free.... Answered my question below [1] during cleanup.
}
/* @oldStateDir: @@ -836,17 +826,29 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr mgr, continue; }
So step1 now removes all pcidevs that are on the inactive list and that are unmanaged since "by rule" only a managed device could be on the activeList. I think you need to update the comment for step1 in any case since there's no removal from the activeList.
+ i++; + } + + /* Step 2: Move devices from the active list to the inactive list */ + for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { + virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i); + virPCIDevicePtr actual; + VIR_DEBUG("Removing PCI device %s from active list", virPCIDeviceGetName(pci)); - virPCIDeviceListDel(mgr->activePCIHostdevs, pci); - i++; + actual = virPCIDeviceListSteal(mgr->activePCIHostdevs, pci); + + if (actual) { + VIR_DEBUG("Adding PCI device %s to inactive list", + virPCIDeviceGetName(pci)); + virPCIDeviceListAdd(mgr->inactivePCIHostdevs, actual);
Coverity notes that there's no error checking here like there is in other calls to virPCIDeviceListAdd. Perhaps similar to step 5 in prepare, we could use VIR_WARN. I also think it might be worthwhile to make a common routine to perform this step - the only difference being perhaps a 'return/fail on error' flag that would be set for Prepare and clear for ReAttach
+ } }
- /* At this point, any device that had been used by the guest is in - * pcidevs, but has been removed from activePCIHostdevs. - */ + /* At this point, any device that had been used by the guest has been + * moved to the inactive list */
So yes, step 3 makes perfect sense now.
- /* Step 2: restore original network config of hostdevs that used + /* Step 3: restore original network config of hostdevs that used * <interface type='hostdev'> */ for (i = 0; i < nhostdevs; i++) { @@ -871,7 +873,7 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr mgr, } }
- /* Step 3: perform a PCI Reset on all devices */ + /* Step 4: perform a PCI Reset on all devices */ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i);
@@ -888,16 +890,26 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr mgr, } }
- /* Step 4: reattach devices to their host drivers (if managed) or place - * them on the inactive list (if not managed) - */ - while (virPCIDeviceListCount(pcidevs) > 0) { - virPCIDevicePtr pci = virPCIDeviceListStealIndex(pcidevs, 0); - virHostdevReattachPCIDevice(mgr, pci); + /* Step 5: Reattach managed devices to their host drivers; unmanaged + * devices don't need to be processed further */
Why use pcidevs? Why not just walk inactivePCIHostdevs?
+ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { + virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i); + virPCIDevicePtr actual; + + /* We need to look up the actual device because that's what + * virHostdevReattachPCIDevice() expects as its argument */ + if (!(actual = virPCIDeviceListFind(mgr->inactivePCIHostdevs, pci))) + continue; + + if (virPCIDeviceGetManaged(actual)) + virHostdevReattachPCIDevice(mgr, actual); + else + VIR_DEBUG("Not reattaching unmanaged PCI device %s", + virPCIDeviceGetName(actual));
If I've read things right - there shouldn't be any unmanaged device in pcidevs at this point as they should have been removed during step1. In any case, if you walk inactiveList, then all this is moot. Also now that we're no longer managing the device it should be removed from inactive and free'd. IOW: THis loop becomes a while inactivePCIHostdevs has elements, call Reattach, then free the device.
}
- virObjectUnref(pcidevs); cleanup:
[1] There could be a memory leak here - I think we need to walk the remaining pcidevs entry and free each entry before unreffing the containing object. I think that's what that the previous step4 would do with the removed virPCIDeviceFree after the ListStealIndex.
+ virObjectUnref(pcidevs); virObjectUnlock(mgr->activePCIHostdevs); virObjectUnlock(mgr->inactivePCIHostdevs); }
On Sun, 2016-03-13 at 10:06 -0400, John Ferlan wrote:
After this patch, ownership of virPCIDevice instances is very easy to keep track of: for each host PCI device, the only instance that actually matters is the one inside one of the bookkeeping list. Whenever some operation needs to be performed on a PCI device, the actual device is looked up first; when this is not the case, a comment explains the reason. --- src/util/virhostdev.c | 118 +++++++++++++++++++++++++++----------------------- 1 file changed, 65 insertions(+), 53 deletions(-) FWIW: from your cover - splitting this up... I think it's possible that
On 03/07/2016 12:24 PM, Andrea Bolognani wrote: the changes for calling virHostdevReattachPCIDevice with an actual device could be in a separate patch... But I certainly see how this and that are "tied" together especially if you're considering the failure path through reattachdevs where you've definitely put things on the inactiveList. There may also be a 4 patches overall in here since I see the Reattach code could place an unmanaged device on the inactiveList (which you modify in this patch) and I think the ReAttach code adjustments could be separate too.
I'll be honest: if we can agree that the code, with this patch applied, is correct, I see little value in splitting it further. It's not like we're really going to want to have just *some* of these changes but not the rest... They're very much connected.
I think you're on the right track, with all these changes - it's some minor details. I've been looking at the changes, walking away, looking at them, so hopefully the following is followable! BTW: I chose not to look at the last 2 patches as I think there are too many questions left over
Fair enough :) But you'll see that those patches are absolutely trivial - this patch right here is the whole point of the series, and it's all smooth sailing from here on :)
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index d1529c5..b2f54cd 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -618,28 +618,22 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, last_processed_hostdev_vf = i; } - /* Step 5: Now mark all the devices as active */ + /* Step 5: Move devices from the inactive list to the active list */ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i); + virPCIDevicePtr actual; - VIR_DEBUG("Adding PCI device %s to active list", + VIR_DEBUG("Removing PCI device %s from inactive list", virPCIDeviceGetName(pci)); - if (virPCIDeviceListAdd(mgr->activePCIHostdevs, pci) < 0) - goto inactivedevs; - } - - /* Step 6: Now remove the devices from inactive list. */ - for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { - virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i); + actual = virPCIDeviceListSteal(mgr->inactivePCIHostdevs, pci); - VIR_DEBUG("Removing PCI device %s from inactive list", + VIR_DEBUG("Adding PCI device %s to active list", virPCIDeviceGetName(pci)); - virPCIDeviceListDel(mgr->inactivePCIHostdevs, pci); + if (!actual || virPCIDeviceListAdd(mgr->activePCIHostdevs, actual) < 0) + goto inactivedevs; } This is so much more logical to me than the previous code.
Yay :)
One question though - why not use virPCIDeviceListStealIndex? Theoretically the inactiveList and the pcidevs list is the same right? So for every devices on pcidevs - steal index 0 of inactiveList and place it on activeList. It's just a slight optimization.
This is in no way guaranteed to be the case. The inactive list keeps track of all inactive devices, including the ones that have manually detached from the host by the user and that we have no intention of ever attaching to a guest. Consider the case of an Ethernet adapter with 4 ports that are presented as 4 separate PCI devices, all in the same IOMMU group: if the user wants to pass only a single port to the guest, it will detach all of them from the host using 'virsh nodedev-detach' or similar, and then call 'virsh attach-device' a single time. The three remaining devices will stay in the inactive list until the user calls 'virsh nodedev-reattach' on them. Another example: the user might decide, for whatever reason, to detach a PCI device from the host, and then proceed to assign a completely unrelated host PCI device to a guest, leaving the previous one unassigned.
- /* Step 7: Now set the used_by_domain of the device in - * activePCIHostdevs as domain name. - */ + /* Step 6: Set driver and domain information */ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr pci, actual; @@ -655,7 +649,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, virPCIDeviceSetUsedBy(actual, drv_name, dom_name); } - /* Step 8: Now set the original states for hostdev def */ + /* Step 7: Now set the original states for hostdev def */ for (i = 0; i < nhostdevs; i++) { virPCIDevicePtr actual; virDomainHostdevDefPtr hostdev = hostdevs[i]; @@ -690,23 +684,25 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, } } - /* Step 9: Now steal all the devices from pcidevs */ - while (virPCIDeviceListCount(pcidevs) > 0) - virPCIDeviceListStealIndex(pcidevs, 0); - Help me out... Why were these lines removed? The inactiveDevs is initially a copy of whatever is on 'pcidevs'... Thus activeDevs takes from there and not pcidevs, right?
Before the changes, objects from 'pcidevs' were *added* to the active list, so it was critical that 'pcidevs' was emptied here: if that didn't happen, the call to virObjectUnref() would have destroyed not only the device list, but the devices themselves, rendering the contents of the active list partially invalid. After the changes, the objects in 'pcidevs' are *always* disposable: when devices are detached from the host, they are *copied* instead of added to the inactive list, and later on those *copies* are *moved* to the active list. So we no longer need to be careful here: all the data we care about is either in the active list or the inactive list, and 'pcidevs', along with all its contents, can be released on a whim in the cleanup path. There's more on this below.
ret = 0; goto cleanup; inactivedevs: - /* Only steal all the devices from activePCIHostdevs. We will - * free them in virObjectUnref(). - */ + /* Move devices back to the inactive list so that they can be + * processed properly below (reattachdevs label) */ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i); + virPCIDevicePtr actual; VIR_DEBUG("Removing PCI device %s from active list", virPCIDeviceGetName(pci)); - virPCIDeviceListSteal(mgr->activePCIHostdevs, pci); + if (!(actual = virPCIDeviceListSteal(mgr->activePCIHostdevs, pci))) + continue; + + VIR_DEBUG("Adding PCI device %s to inactive list", + virPCIDeviceGetName(pci)); + if (virPCIDeviceListAdd(mgr->inactivePCIHostdevs, actual) < 0) + continue; Another option is to VIR_WARN (INFO, DEBUG, whatever) the failure. You may also want to clear the error as well (virResetLastError). There are two possible errors - it already exists on the list and memory allocation errors.
The first one would not really be a failure, though - we might have processed only part of the devices before encountering an error, so it's totally possible that some of them have not been added to the active list yet. As for the second one... There really isn't much you can do if your memory allocation fails and you're already in the error path, is there? :(
The continue is superfluous since this is the end of the loop - it only matters if someone adds more checks afterwards
I'd rather keep it for clarity's sake.
resetvfnetconfig: @@ -716,16 +712,22 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, reattachdevs: for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i); + virPCIDevicePtr actual; - if (virPCIDeviceGetManaged(pci)) { + /* We need to look up the actual device because that's what + * virPCIDeviceReattach() exepects as its argument */ "expects"
Right.
As I read it virPCIDeviceReattach look for the device on the activeList, but will "blindly" remove from the inactiveList *if* provided. That's only true for xen_driver and virpcitest. I believe this process is the more correct one.
virPCIDeviceReattach() only checks whether the device is on the active list for safety purposes; you still need to pass in an actual device (eg. instance taken from one of the bookkeeping lists, the inactive list in this case) because virPCIDeviceUnbindFromStub(), which performs the actual task of reattaching the device to the host, needs to have information such as unbind_from_stub, remove_slot and reprobe, which had been stored in the actual device by virPCIDeviceDetach().
+ if (!(actual = virPCIDeviceListFind(mgr->inactivePCIHostdevs, pci))) + continue; + + if (virPCIDeviceGetManaged(actual)) { VIR_DEBUG("Reattaching managed PCI device %s", - virPCIDeviceGetName(pci)); - ignore_value(virPCIDeviceReattach(pci, + virPCIDeviceGetName(actual)); + ignore_value(virPCIDeviceReattach(actual, mgr->activePCIHostdevs, mgr->inactivePCIHostdevs)); } else { VIR_DEBUG("Not reattaching unmanaged PCI device %s", - virPCIDeviceGetName(pci)); + virPCIDeviceGetName(actual)); } } @@ -745,17 +747,6 @@ static void virHostdevReattachPCIDevice(virHostdevManagerPtr mgr, virPCIDevicePtr actual) { - /* If the device is not managed and was attached to guest - * successfully, it must have been inactive. - */ - if (!virPCIDeviceGetManaged(actual)) { - VIR_DEBUG("Adding unmanaged PCI device %s to inactive list", - virPCIDeviceGetName(actual)); - if (virPCIDeviceListAdd(mgr->inactivePCIHostdevs, actual) < 0) - virPCIDeviceFree(actual); - return; - } - Is it just me getting really confused - this code used to add devices to the inactiveList even though (at some point in time) the Prepare code would "drop" them? E.G. step 2 before any of these changes. Was this perhaps what caused you to add that PCIDeviceListFind code in step 2 of patch 21 that I questioned? Perhaps this alone deserves it's own patch and I think is it's own problem since the lookaside lists are for devices that are managed, right?
This used to take care of the unmanaged devices by adding them to the inactive list and returning early to skip the call to virPCIDeviceReattach() later on. Now we take care of that in the caller, ReAttachPCIDevices(), by moving *all devices* from the active list to the inactive list, regardless of whether they were managed or not, and only calling ReattachPCIDevice() on managed devices. The end result is the same.
/* Wait for device cleanup if it is qemu/kvm */ if (virPCIDeviceGetStubDriver(actual) == VIR_PCI_STUB_DRIVER_KVM) { int retries = 100; @@ -774,7 +765,6 @@ virHostdevReattachPCIDevice(virHostdevManagerPtr mgr, err ? err->message : _("unknown error")); virResetError(err); } - virPCIDeviceFree(actual); Hmm... mental note - what changes here... to cause us not to need to free.... Answered my question below [1] during cleanup.
My reply is below as well :)
} /* @oldStateDir: @@ -836,17 +826,29 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr mgr, continue; } So step1 now removes all pcidevs that are on the inactive list and that are unmanaged since "by rule" only a managed device could be on the activeList.
There is no such rule or assumptions: both managed and unmanaged devices belong to the active list. The only criteria for being on the active list is, well, being active, ie. assigned to a guest. After the changes, step 1 makes sure that 'pcidevs' (our working set) only contains (pointers to) devices that were both *active* and *used by the correct domain and driver* to begin with, which is what we expect to be dealing with in the rest of the function. Actually, this was already the case as of patch 15, and the logic was the same even before that. The only change here is splitting the code that changes / validates 'pcidevs' from the one that changes the contents of the active list.
I think you need to update the comment for step1 in any case since there's no removal from the activeList.
Yeah, thanks for spotting that.
+ i++; + } + + /* Step 2: Move devices from the active list to the inactive list */ + for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { + virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i); + virPCIDevicePtr actual; + VIR_DEBUG("Removing PCI device %s from active list", virPCIDeviceGetName(pci)); - virPCIDeviceListDel(mgr->activePCIHostdevs, pci); - i++; + actual = virPCIDeviceListSteal(mgr->activePCIHostdevs, pci); + + if (actual) { + VIR_DEBUG("Adding PCI device %s to inactive list", + virPCIDeviceGetName(pci)); + virPCIDeviceListAdd(mgr->inactivePCIHostdevs, actual); Coverity notes that there's no error checking here like there is in other calls to virPCIDeviceListAdd. Perhaps similar to step 5 in prepare, we could use VIR_WARN.
I'll add error checking.
I also think it might be worthwhile to make a common routine to perform this step - the only difference being perhaps a 'return/fail on error' flag that would be set for Prepare and clear for ReAttach
This might be a nice idea for further improvement, although it might not matter as much once the code is refactored as originally outlined in [1], which is going to be my next step.
- /* At this point, any device that had been used by the guest is in - * pcidevs, but has been removed from activePCIHostdevs. - */ + /* At this point, any device that had been used by the guest has been + * moved to the inactive list */ So yes, step 3 makes perfect sense now.
Yay :)
- /* Step 2: restore original network config of hostdevs that used + /* Step 3: restore original network config of hostdevs that used * <interface type='hostdev'> */ for (i = 0; i < nhostdevs; i++) { @@ -871,7 +873,7 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr mgr, } } - /* Step 3: perform a PCI Reset on all devices */ + /* Step 4: perform a PCI Reset on all devices */ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i); @@ -888,16 +890,26 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr mgr, } } - /* Step 4: reattach devices to their host drivers (if managed) or place - * them on the inactive list (if not managed) - */ - while (virPCIDeviceListCount(pcidevs) > 0) { - virPCIDevicePtr pci = virPCIDeviceListStealIndex(pcidevs, 0); - virHostdevReattachPCIDevice(mgr, pci); + /* Step 5: Reattach managed devices to their host drivers; unmanaged + * devices don't need to be processed further */ Why use pcidevs? Why not just walk inactivePCIHostdevs?
For consistency, and because as explained above some of the devices in the inactive list might be completely unrelated to the task at hand.
+ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { + virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i); + virPCIDevicePtr actual; + + /* We need to look up the actual device because that's what + * virHostdevReattachPCIDevice() expects as its argument */ + if (!(actual = virPCIDeviceListFind(mgr->inactivePCIHostdevs, pci))) + continue; + + if (virPCIDeviceGetManaged(actual)) + virHostdevReattachPCIDevice(mgr, actual); + else + VIR_DEBUG("Not reattaching unmanaged PCI device %s", + virPCIDeviceGetName(actual)); If I've read things right - there shouldn't be any unmanaged device in pcidevs at this point as they should have been removed during step1. In any case, if you walk inactiveList, then all this is moot.
Not true, there can definitely be unmanaged devices. And we can't just loop over the inactive list because of the reasons explained above.
Also now that we're no longer managing the device it should be removed from inactive and free'd. IOW: THis loop becomes a while inactivePCIHostdevs has elements, call Reattach, then free the device.
virHostdevReattachPCIDevice() calls virPCIDeviceReattach(), which takes care of removing the device from the inactive list and releasing the associated memory.
} - virObjectUnref(pcidevs); cleanup: [1] There could be a memory leak here - I think we need to walk the remaining pcidevs entry and free each entry before unreffing the containing object. I think that's what that the previous step4 would do with the removed virPCIDeviceFree after the ListStealIndex.
There is no memory leak: virObjectUnref() is now called unconditionally on 'pcidevs', and that takes care of releasing both the container itself *and* the instances that had been added to it. As explained above, the original code allowed the same instance to be part of two different lists at a time, 'pcidevs' and one of the bookkeeping lists. (Both of them, in some cases, but that was just for a limited period of time during state transitions.) What the new code does is regard all objects in 'pcidevs' as eminently disposable, and store the actual objects in the bookkeeping lists *every single time*. Each virPCIDevice instance, be it valuable or not, is only ever in a single list at a time. The downside is that we have to copy objects around slightly more frequently - even though some other changes might very well offset that overhead already - but the upside is that we can be way more ham-fisted when it comes to deleting stuff, which is exactly what we're doing here. It doesn't matter what's inside 'pcidevs', it's always going to be stuff we already have an authoritative copy of somewhere else, so let's just release it in the cleanup path and forget about it. Way easier to reason about, at least for my brain :)
+ virObjectUnref(pcidevs); virObjectUnlock(mgr->activePCIHostdevs); virObjectUnlock(mgr->inactivePCIHostdevs); }
Thanks for getting this far - keeping a message this long understandable is pretty challenging, hopefully I've done a somewhat decent job at it. I believe there was some disconnect between the role I intended a variable to have and the role you understood they would have, and that caused lots of head scratching on your part... This is something I tried to address with patch 19, but I can see it was not as effective as I hoped it would be. Any suggestions on how to improve the situation? Cheers. [1] https://www.redhat.com/archives/libvir-list/2016-January/msg01066.html -- Andrea Bolognani Software Engineer - Virtualization Team
[...]
- /* Step 5: Now mark all the devices as active */ + /* Step 5: Move devices from the inactive list to the active list */ for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i); + virPCIDevicePtr actual;
- VIR_DEBUG("Adding PCI device %s to active list", + VIR_DEBUG("Removing PCI device %s from inactive list", virPCIDeviceGetName(pci)); - if (virPCIDeviceListAdd(mgr->activePCIHostdevs, pci) < 0) - goto inactivedevs; - } - - /* Step 6: Now remove the devices from inactive list. */ - for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { - virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i); + actual = virPCIDeviceListSteal(mgr->inactivePCIHostdevs, pci);
- VIR_DEBUG("Removing PCI device %s from inactive list", + VIR_DEBUG("Adding PCI device %s to active list", virPCIDeviceGetName(pci)); - virPCIDeviceListDel(mgr->inactivePCIHostdevs, pci); + if (!actual || virPCIDeviceListAdd(mgr->activePCIHostdevs, actual) < 0) + goto inactivedevs; }
This is so much more logical to me than the previous code.
Yay :)
While writing the tomb for patch 20, I had one question/issue... You could have already addressed in your upcoming patches, but I'll ask anyway before I see them.... Essentially combining step5/step6 will *steal* from the inactiveList (the rich) and give to the activeList (the poor). That's not a problem. But, doesn't this mean only managed devices are moved to activeList as opposed to all possible hostdevs (in pcidevs) - at least during Prepare? Again it's this whole how does the inactiveList get populated conundrum (vis-a-vis devices in the guest XML that are not managed). Assuming the Reattach code will no longer place unmanaged devices on the inactiveList the point/question is probably moot! John
On Wed, 2016-03-16 at 15:33 -0400, John Ferlan wrote:
While writing the tomb for patch 20, I had one question/issue... You could have already addressed in your upcoming patches, but I'll ask anyway before I see them....
Essentially combining step5/step6 will *steal* from the inactiveList (the rich) and give to the activeList (the poor). That's not a problem.
But, doesn't this mean only managed devices are moved to activeList as opposed to all possible hostdevs (in pcidevs) - at least during Prepare? Again it's this whole how does the inactiveList get populated conundrum (vis-a-vis devices in the guest XML that are not managed).
Should be addressed by the small spin-off series I posted yesterday :) Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
Instead of forcing the values for the unbind_from_stub, remove_slot and reprobe properties, look up the actual device and use that when calling virPCIDeviceReattach(). This ensures the device is restored to its original state after reattach: for example, if it was not bound to any driver before detach, it will not be bound forcefully during reattach. --- src/util/virhostdev.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index b2f54cd..86a0331 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -1613,6 +1613,7 @@ virHostdevPCINodeDeviceReAttach(virHostdevManagerPtr mgr, virPCIDevicePtr pci) { struct virHostdevIsPCINodeDeviceUsedData data = { mgr, NULL, false }; + virPCIDevicePtr actual; int ret = -1; virObjectLock(mgr->activePCIHostdevs); @@ -1621,11 +1622,12 @@ virHostdevPCINodeDeviceReAttach(virHostdevManagerPtr mgr, if (virHostdevIsPCINodeDeviceUsed(virPCIDeviceGetAddress(pci), &data)) goto cleanup; - virPCIDeviceSetUnbindFromStub(pci, true); - virPCIDeviceSetRemoveSlot(pci, true); - virPCIDeviceSetReprobe(pci, true); + /* We need to look up the actual device because that's what + * virPCIDeviceReattach() expects as its argument */ + if (!(actual = virPCIDeviceListFind(mgr->inactivePCIHostdevs, pci))) + goto cleanup; - if (virPCIDeviceReattach(pci, mgr->activePCIHostdevs, + if (virPCIDeviceReattach(actual, mgr->activePCIHostdevs, mgr->inactivePCIHostdevs) < 0) goto cleanup; -- 2.5.0
Ensure the code behaves properly even for situations that were not being considered before, such as simply detaching devices from the host without attaching them to a guest and attaching devices as managed even though they had already been manually detached from the host. --- tests/virhostdevtest.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 78 insertions(+), 6 deletions(-) diff --git a/tests/virhostdevtest.c b/tests/virhostdevtest.c index c0ab044..617e86c 100644 --- a/tests/virhostdevtest.c +++ b/tests/virhostdevtest.c @@ -253,7 +253,7 @@ testVirHostdevReAttachPCIHostdevs_unmanaged(void) } static int -testVirHostdevPreparePCIHostdevs_managed(void) +testVirHostdevPreparePCIHostdevs_managed(bool mixed) { int ret = -1; size_t active_count, inactive_count, i; @@ -270,7 +270,13 @@ testVirHostdevPreparePCIHostdevs_managed(void) hostdevs, nhostdevs, 0) < 0) goto cleanup; CHECK_LIST_COUNT(mgr->activePCIHostdevs, active_count + nhostdevs); - CHECK_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); + /* If testing a mixed roundtrip, devices are already in the inactive list + * before we start and are removed from it as soon as we attach them to + * the guest */ + if (mixed) + CHECK_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count - nhostdevs); + else + CHECK_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); /* Test conflict */ active_count = virPCIDeviceListCount(mgr->activePCIHostdevs); @@ -304,7 +310,7 @@ testVirHostdevPreparePCIHostdevs_managed(void) } static int -testVirHostdevReAttachPCIHostdevs_managed(void) +testVirHostdevReAttachPCIHostdevs_managed(bool mixed) { int ret = -1; size_t active_count, inactive_count, i; @@ -328,7 +334,12 @@ testVirHostdevReAttachPCIHostdevs_managed(void) virHostdevReAttachPCIDevices(mgr, drv_name, dom_name, hostdevs, nhostdevs, NULL); CHECK_LIST_COUNT(mgr->activePCIHostdevs, active_count - nhostdevs); - CHECK_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); + /* If testing a mixed roundtrip, devices are added back to the inactive + * list as soon as we detach from the guest */ + if (mixed) + CHECK_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count + nhostdevs); + else + CHECK_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count); ret = 0; @@ -432,6 +443,31 @@ testVirHostdevUpdateActivePCIHostdevs(void) } /** + * testVirHostdevRoundtripNoGuest: + * @opaque: unused + * + * Perform a roundtrip without ever assigning devices to the guest. + * + * 1. Detach devices from the host + * 2. Reattach devices to the host + */ +static int +testVirHostdevRoundtripNoGuest(const void *opaque ATTRIBUTE_UNUSED) +{ + int ret = -1; + + if (testVirHostdevDetachPCINodeDevice() < 0) + goto out; + if (testVirHostdevReAttachPCINodeDevice() < 0) + goto out; + + ret = 0; + + out: + return ret; +} + +/** * testVirHostdevRoundtripUnmanaged: * @opaque: unused * @@ -479,9 +515,9 @@ testVirHostdevRoundtripManaged(const void *opaque ATTRIBUTE_UNUSED) int ret = -1; if (virHostdevHostSupportsPassthroughKVM()) { - if (testVirHostdevPreparePCIHostdevs_managed() < 0) + if (testVirHostdevPreparePCIHostdevs_managed(false) < 0) goto out; - if (testVirHostdevReAttachPCIHostdevs_managed() < 0) + if (testVirHostdevReAttachPCIHostdevs_managed(false) < 0) goto out; } @@ -492,6 +528,40 @@ testVirHostdevRoundtripManaged(const void *opaque ATTRIBUTE_UNUSED) } /** + * testVirHostdevRoundtripMixed: + * @opaque: unused + * + * Perform a roundtrip with managed devices but manually detach the devices + * from the host first. + * + * 1. Detach devices from the host + * 2. Attach devices to the guest as managed + * 3. Detach devices from the guest as managed + * 4. Reattach devices to the host + */ +static int +testVirHostdevRoundtripMixed(const void *opaque ATTRIBUTE_UNUSED) +{ + int ret = -1; + + if (testVirHostdevDetachPCINodeDevice() < 0) + goto out; + if (virHostdevHostSupportsPassthroughKVM()) { + if (testVirHostdevPreparePCIHostdevs_managed(true) < 0) + goto out; + if (testVirHostdevReAttachPCIHostdevs_managed(true) < 0) + goto out; + } + if (testVirHostdevReAttachPCINodeDevice() < 0) + goto out; + + ret = 0; + + out: + return ret; +} + +/** * testVirHostdevOther: * @opaque: unused * @@ -546,8 +616,10 @@ mymain(void) if (myInit() < 0) fprintf(stderr, "Init data structures failed."); + DO_TEST(testVirHostdevRoundtripNoGuest); DO_TEST(testVirHostdevRoundtripUnmanaged); DO_TEST(testVirHostdevRoundtripManaged); + DO_TEST(testVirHostdevRoundtripMixed); DO_TEST(testVirHostdevOther); myCleanup(); -- 2.5.0
participants (3)
-
Andrea Bolognani -
John Ferlan -
Laine Stump