[libvirt] [PATCH 0/3] libvirt: Fix automatic SCSI controller create in hotplug
Trying to hotplug a SCSI device to a domain without the required SCSI controller fails. For hostdev SCSI device hotplug commits 0d8b24f6 and 0785966d rearranged the automatic creation of the required SCSI controllers from the parsing xml code into the post parsing code section of virDomainDefParseXML. In doing so the code will create but not hotplug the missing SCSI controller and on the hotplug path thru the same code the SCSI controller will not get hotplugged since it already exits. Then the SCSI hostdev device is hotplugged and runs into the following internal error error: Failed to attach device from scsidev.xml error: internal error: Device alias was not set for scsi controller with index 0 For disk SCSI device hotplug commit 0260506c added in method qemuBuildDriveDevStr a lookup of the controller alias. The internal error error: Failed to attach device from scsi_disk.xml error: internal error: Could not find scsi controller with index 0 required for device occurs because in method qemuDomainAttachSCSIDisk the automatic creation of the missing SCSI controller occurs after calling qemuBuildDriveDevStr. Boris Fiuczynski (3): conf: Reposition adding SCSI controller for SCSI hostdev hotplug Revert "conf: Try controller add when searching hostdev bus for unit" Automatic SCSI controller creation in SCSI disk hotplug broken src/conf/domain_conf.c | 16 ++++------------ src/qemu/qemu_hotplug.c | 12 ++++++------ 2 files changed, 10 insertions(+), 18 deletions(-) -- 2.3.0
This patch reverts a part of commit 0d8b24f6b. With commits 0d8b24g6 and 0785966d automatically adding a required controller had been moved from the domain xml parsing code section into the device post xml parsing code section and in doing so breaking as a side effect the SCSI hostdev hotplugging in case the required SCSI controller had not been defined in the domain. This behavior occurs because the SCSI controller gets added but is NOT hotplugged. In the hotplug code section the controller is searched for and if not found would be hotplugged but since a SCSI controller already exists in the domain defintion the required SCSI controller hotplug is never executed. This results later in the internal error: error: Failed to attach device from st0.xml error: internal error: Device alias was not set for scsi controller with index 0 This patch moves the automatic add of a SCSI controller for a SCSI hostdev device back into the domain xml parsing code section. Signed-off-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Reviewed-by: Stefan Zimmermann <stzi@linux.vnet.ibm.com> --- src/conf/domain_conf.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index cbfc41e..69cfd0f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -16285,6 +16285,9 @@ virDomainDefParseXML(xmlDocPtr xml, } def->hostdevs[def->nhostdevs++] = hostdev; + + if (virDomainDefMaybeAddHostdevSCSIcontroller(def) < 0) + goto error; } VIR_FREE(nodes); -- 2.3.0
On 11/30/2015 06:05 AM, Boris Fiuczynski wrote:
This patch reverts a part of commit 0d8b24f6b. With commits 0d8b24g6 and 0785966d automatically adding a required controller
g6 ? I assume you meant f6b...
had been moved from the domain xml parsing code section into the device post xml parsing code section and in doing so breaking as a side effect the SCSI hostdev hotplugging in case the required SCSI controller had not been defined in the domain.
What side effect? What was missed by moving the controller add to post processing?
This behavior occurs because the SCSI controller gets added but is NOT hotplugged. In the hotplug code section the controller is searched for and if not found would be hotplugged but since a SCSI controller already exists in the domain defintion the required SCSI controller hotplug is never executed.
s/defintion/definition Hmmm, is this the side effect? It perhaps would have been helpful to know what API in the hotplug section of code you're referencing. Are you referencing qemuDomainFindOrCreateSCSIDiskController?
This results later in the internal error:
error: Failed to attach device from st0.xml error: internal error: Device alias was not set for scsi controller with index 0
I can reproduce this issue if I define/start a domain with no SCSI controllers. Then whether the added hostdev has a drive address or not, the hotplug fails. I also get the same results for disk hotplug, so perhaps the issue needs a more "generic" solution.
This patch moves the automatic add of a SCSI controller for a SCSI hostdev device back into the domain xml parsing code section.
I'm still baffled why this "works", but need some time to work through the algorithms.
Signed-off-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Reviewed-by: Stefan Zimmermann <stzi@linux.vnet.ibm.com> --- src/conf/domain_conf.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index cbfc41e..69cfd0f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -16285,6 +16285,9 @@ virDomainDefParseXML(xmlDocPtr xml, }
def->hostdevs[def->nhostdevs++] = hostdev; + + if (virDomainDefMaybeAddHostdevSCSIcontroller(def) < 0) + goto error;
What if the hotplug <hostdev> xml provided an address using perhaps controller==1? I think this may only work in the default case and/or when controller==0 in the provided an address case. If we were to go ahead with this patch, I'd have to merge in patch2 as well. That way a git bisect that falls in between patch 1 and 2 won't cause some "other" issue... I'm still trying to "internalize" the whole issue, this is where I'm at. Prior to any of my changes, at parse processing if a <hostdev> didn't have an <address> element, it would be added. Then nhostdevs would be incremented, and virDomainDefMaybeAddHostdevSCSIcontroller would be called. That code may call virDomainDefMaybeAddController or return 0 (and not call virDomainDefMaybeAddController). With my changes, address assignment is deferred to post processing as well as maybe adding a controller. At post processing, for a scsi hostdev without an address defined, virDomainHostdevAssignAddress is called which would call virDomainDefMaybeAddController if one wasn't found or one was found, but was full. That would seemingly add the controller via virDomainDefMaybeAddController as would the virDomainDefMaybeAddHostdevSCSIcontroller. But it seems perhaps for some reason it shouldn't or didn't. Let's say it didn't add it (for whatever reason), it seems the expectation is that when qemuDomainFindOrCreateSCSIDiskController is called it won't find the controller present and create/add it, which I think is the side effect referenced in the commit message. I think I'm just missing some nuance, but I'll keep poking to figure it out. I at least wanted to provide some feedback to ensure this wasn't reviewed/pushed by someone else! Although if someone else is looking at it and has some ideas that'd be great too. John
} VIR_FREE(nodes);
On 12/01/2015 05:21 PM, John Ferlan wrote:
On 11/30/2015 06:05 AM, Boris Fiuczynski wrote:
This patch reverts a part of commit 0d8b24f6b. With commits 0d8b24g6 and 0785966d automatically adding a required controller
g6 ? I assume you meant f6b...
yes
had been moved from the domain xml parsing code section into the device post xml parsing code section and in doing so breaking as a side effect the SCSI hostdev hotplugging in case the required SCSI controller had not been defined in the domain.
What side effect?
The side effect is that the change broke SCSI device hotplug if the domain does not have the SCSI controller defined. I thought the sentence above covered that. What was missed by moving the controller add to post
processing? I guess you missed when moving that you moved from domain parsing to device parsing.
This behavior occurs because the SCSI controller gets added but is NOT hotplugged. In the hotplug code section the controller is searched for and if not found would be hotplugged but since a SCSI controller already exists in the domain defintion the required SCSI controller hotplug is never executed.
s/defintion/definition
ok
Hmmm, is this the side effect? It perhaps would have been helpful to know what API in the hotplug section of code you're referencing. Are you referencing qemuDomainFindOrCreateSCSIDiskController?
Hotplug uses the same device parsing that also domain parsing uses!
This results later in the internal error:
error: Failed to attach device from st0.xml error: internal error: Device alias was not set for scsi controller with index 0
I can reproduce this issue if I define/start a domain with no SCSI controllers. Then whether the added hostdev has a drive address or not, the hotplug fails. I also get the same results for disk hotplug, so perhaps the issue needs a more "generic" solution.
As far as I can see there is no generic solution in this case.
This patch moves the automatic add of a SCSI controller for a SCSI hostdev device back into the domain xml parsing code section.
I'm still baffled why this "works", but need some time to work through the algorithms.
I also had a hard baffled time until I found the code shift was from domain to device ...
Signed-off-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Reviewed-by: Stefan Zimmermann <stzi@linux.vnet.ibm.com> --- src/conf/domain_conf.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index cbfc41e..69cfd0f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -16285,6 +16285,9 @@ virDomainDefParseXML(xmlDocPtr xml, }
def->hostdevs[def->nhostdevs++] = hostdev; + + if (virDomainDefMaybeAddHostdevSCSIcontroller(def) < 0) + goto error;
What if the hotplug <hostdev> xml provided an address using perhaps controller==1? I think this may only work in the default case and/or when controller==0 in the provided an address case.
If we were to go ahead with this patch, I'd have to merge in patch2 as well. That way a git bisect that falls in between patch 1 and 2 won't cause some "other" issue...
Yes, we can merge it for logical means. Leaving them separate should not hurt since patch 1 adds first the "add controller" on domain parsing back again and patch 2 removes the "add controller" from the device parsing. But you are correct that only with patch 1 and 2 applied the problem is resolved.
I'm still trying to "internalize" the whole issue, this is where I'm at.
Prior to any of my changes, at parse processing if a <hostdev> didn't have an <address> element, it would be added. Then nhostdevs would be incremented, and virDomainDefMaybeAddHostdevSCSIcontroller would be called. That code may call virDomainDefMaybeAddController or return 0 (and not call virDomainDefMaybeAddController).
With my changes, address assignment is deferred to post processing as well as maybe adding a controller. At post processing, for a scsi hostdev without an address defined, virDomainHostdevAssignAddress is called which would call virDomainDefMaybeAddController if one wasn't found or one was found, but was full. That would seemingly add the controller via virDomainDefMaybeAddController as would the virDomainDefMaybeAddHostdevSCSIcontroller. But it seems perhaps for some reason it shouldn't or didn't.
Let's say it didn't add it (for whatever reason), it seems the expectation is that when qemuDomainFindOrCreateSCSIDiskController is called it won't find the controller present and create/add it, which I think is the side effect referenced in the commit message.
I think I'm just missing some nuance, but I'll keep poking to figure it out. I at least wanted to provide some feedback to ensure this wasn't reviewed/pushed by someone else! Although if someone else is looking at it and has some ideas that'd be great too.
John
} VIR_FREE(nodes);
-- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martina Köderitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On 11/30/2015 06:05 AM, Boris Fiuczynski wrote: [...]
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index cbfc41e..69cfd0f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -16285,6 +16285,9 @@ virDomainDefParseXML(xmlDocPtr xml, }
def->hostdevs[def->nhostdevs++] = hostdev; + + if (virDomainDefMaybeAddHostdevSCSIcontroller(def) < 0) + goto error; } VIR_FREE(nodes);
After some more experimentation and making sure my debug environment was "up to date" (e.g. had virtlogd started)... Using top of tree I can hotplug a hostdev as long as it has a drive address defined. Not sure why it was failing before, but at least things are making more sense now. The reason why that path works with top of tree is because virDomainDeviceDefPostParseInternal won't call virDomainHostdevAssignAddress since hdev->info->type is not VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE. That means when the qemuDomainFindOrCreateSCSIDiskController is called it will do the magic add. The path for this patch handles the inactive domain definition. While patch 2 handles the backend of the active domain path. When you have a running domain, then qemuDomainAttachDeviceFlags (e.g. what virsh attach-device would use) calls virDomainDeviceDefParse which is where the controller is added during virDomainDeviceDefPostParse. So patch 2 where you're removing that PostParse callback is why the hotplug will then work because no longer does post parse add the controller to the domain def. The call to qemuDomainFindOrCreateSCSIDiskController called by qemuDomainAttachSCSIDisk during qemuDomainAttachDeviceDiskLive in the live path of qemuDomainAttachDeviceFlags would then automagically add the controller. Since the active path wouldn't have added it during post parse. While these patches would work for the active domain hostdev add, I think perhaps a different mechanism should be used. Given the failure from trying to add a disk in the same environment, I'm not sure either patch solves the root problem that during hotplug we're relying on *not* adding a controller device when a new one is deemed necessary (for both hostdev and disk). One thing that is of interest is that virDomainDefMaybeAddController has 3 return values -1 fail, 0 didn't add a controller, and 1 added a controller. I'm wondering if that can somehow be used to ensure that if we add a controller that we ensure it really gets plugged in. Instead of relying on qemuDomainFindOrCreateSCSIDiskController to backdoor it in. John
On 12/02/2015 01:16 AM, John Ferlan wrote:
On 11/30/2015 06:05 AM, Boris Fiuczynski wrote:
[...]
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index cbfc41e..69cfd0f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -16285,6 +16285,9 @@ virDomainDefParseXML(xmlDocPtr xml, }
def->hostdevs[def->nhostdevs++] = hostdev; + + if (virDomainDefMaybeAddHostdevSCSIcontroller(def) < 0) + goto error; } VIR_FREE(nodes);
After some more experimentation and making sure my debug environment was "up to date" (e.g. had virtlogd started)...
Using top of tree I can hotplug a hostdev as long as it has a drive address defined. Not sure why it was failing before, but at least things are making more sense now. The reason why that path works with top of tree is because virDomainDeviceDefPostParseInternal won't call virDomainHostdevAssignAddress since hdev->info->type is not VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE. That means when the qemuDomainFindOrCreateSCSIDiskController is called it will do the magic add.
The path for this patch handles the inactive domain definition. While patch 2 handles the backend of the active domain path.
Patch 2 is also used in the "inactive domain definition" case. The important difference is that the controller is NOT created on the device definition path so that the controller is added via hotplug by qemuDomainFindOrCreateSCSIDiskController since device hotplug uses the device definition path! That is why this patch reintroduces adding the controller into domain definition that the hotplug code does not use.
When you have a running domain, then qemuDomainAttachDeviceFlags (e.g. what virsh attach-device would use) calls virDomainDeviceDefParse which is where the controller is added during virDomainDeviceDefPostParse. So patch 2 where you're removing that PostParse callback is why the hotplug will then work because no longer does post parse add the controller to the domain def.
Correct.
The call to qemuDomainFindOrCreateSCSIDiskController called by qemuDomainAttachSCSIDisk during qemuDomainAttachDeviceDiskLive in the live path of qemuDomainAttachDeviceFlags would then automagically add the controller. Since the active path wouldn't have added it during post parse.
Correct.
While these patches would work for the active domain hostdev add, I think perhaps a different mechanism should be used. Given the failure from trying to add a disk in the same environment, I'm not sure either patch solves the root problem that during hotplug we're relying on *not* adding a controller device when a new one is deemed necessary (for both hostdev and disk).
I think that the root problem was introduced by moving the adding of the controller from the DOMAIN definition parsing into the DEVICE definition parsing. Since the code at this point seems by its multiple usage patterns rather complex and fragile to changes I choose for this fix to revert the code rather than to reengineer the code and not potentially make things worse.
One thing that is of interest is that virDomainDefMaybeAddController has 3 return values -1 fail, 0 didn't add a controller, and 1 added a controller. I'm wondering if that can somehow be used to ensure that if we add a controller that we ensure it really gets plugged in. Instead of relying on qemuDomainFindOrCreateSCSIDiskController to backdoor it in.
I must say that calling vir > DomainDef < MaybeAddController from the DEVICE parsing and not from the DOMAIN parsing feels rather strange to me. In both cases domain parsing and hotplugging devices the controllers are plugged in AFTER the device parsing is finished! I do not see any backdoor on the hotplug path!
John
-- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martina Köderitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On 12/02/2015 03:27 AM, Boris Fiuczynski wrote:
On 12/02/2015 01:16 AM, John Ferlan wrote:
On 11/30/2015 06:05 AM, Boris Fiuczynski wrote:
[...]
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index cbfc41e..69cfd0f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -16285,6 +16285,9 @@ virDomainDefParseXML(xmlDocPtr xml, }
def->hostdevs[def->nhostdevs++] = hostdev; + + if (virDomainDefMaybeAddHostdevSCSIcontroller(def) < 0) + goto error; } VIR_FREE(nodes);
After some more experimentation and making sure my debug environment was "up to date" (e.g. had virtlogd started)...
Using top of tree I can hotplug a hostdev as long as it has a drive address defined. Not sure why it was failing before, but at least things are making more sense now. The reason why that path works with top of tree is because virDomainDeviceDefPostParseInternal won't call virDomainHostdevAssignAddress since hdev->info->type is not VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE. That means when the qemuDomainFindOrCreateSCSIDiskController is called it will do the magic add.
The path for this patch handles the inactive domain definition. While patch 2 handles the backend of the active domain path.
Patch 2 is also used in the "inactive domain definition" case. The important difference is that the controller is NOT created on the device definition path so that the controller is added via hotplug by qemuDomainFindOrCreateSCSIDiskController since device hotplug uses the device definition path! That is why this patch reintroduces adding the controller into domain definition that the hotplug code does not use.
When you have a running domain, then qemuDomainAttachDeviceFlags (e.g. what virsh attach-device would use) calls virDomainDeviceDefParse which is where the controller is added during virDomainDeviceDefPostParse. So patch 2 where you're removing that PostParse callback is why the hotplug will then work because no longer does post parse add the controller to the domain def.
Correct.
The call to qemuDomainFindOrCreateSCSIDiskController called by qemuDomainAttachSCSIDisk during qemuDomainAttachDeviceDiskLive in the live path of qemuDomainAttachDeviceFlags would then automagically add the controller. Since the active path wouldn't have added it during post parse.
Correct.
While these patches would work for the active domain hostdev add, I think perhaps a different mechanism should be used. Given the failure from trying to add a disk in the same environment, I'm not sure either patch solves the root problem that during hotplug we're relying on *not* adding a controller device when a new one is deemed necessary (for both hostdev and disk).
I think that the root problem was introduced by moving the adding of the controller from the DOMAIN definition parsing into the DEVICE definition parsing.
It's all domain_conf code... virDomainHostdevDefParseXML is either called by virDomainDefParseXML (inactive) or virDomainDeviceDefParse (active, via various drivers). The post parse processing code for driver callbacks is handled more directly through virDomainDeviceDefPostParse at the end of virDomainDeviceDefParse. The post parse processing for inactive callbacks is handled through virDomainDefPostParseDeviceIterator as part of validating the domain in virDomainDefPostParse. IMO it's splitting hairs to claim device processing is that much different than domain processing. It's just that the hotplug code knows how to directly go to the device it wants based on the xml from the attach-device command rather than going through all domain processing. In any case, the existing mechanism to assign an address and thus maybe add a controller for the inactive domain was introduced by commit id 'cdb978955'. That also introduced the code used by the active/hotplug processing. The "side effect" of the code not trying to add a controller was that setting the controller value to something didn't exist (I assume) knowing that CreateSCSIDiskController would do the dirty work later (not documented either which is another "issue" in my mind). Just for completeness... Auto-generating a controller for a hostdev was introduced by commit id '881eb780'. The CreateSCSIDiskController code has been around a lot longer...
Since the code at this point seems by its multiple usage patterns rather complex and fragile to changes I choose for this fix to revert the code rather than to reengineer the code and not potentially make things worse.
I think this may be a short term solution unless I can figure out a better way short term to hotplug an added controller as well if one is created.
One thing that is of interest is that virDomainDefMaybeAddController has 3 return values -1 fail, 0 didn't add a controller, and 1 added a controller. I'm wondering if that can somehow be used to ensure that if we add a controller that we ensure it really gets plugged in. Instead of relying on qemuDomainFindOrCreateSCSIDiskController to backdoor it in.
I must say that calling vir > DomainDef < MaybeAddController from the DEVICE parsing and not from the DOMAIN parsing feels rather strange to me. In both cases domain parsing and hotplugging devices the controllers are plugged in AFTER the device parsing is finished! I do not see any backdoor on the hotplug path!
The "concept" of moving virDomainDefMaybeAddController into virDomainHostdevAssignAddress was I believe meant to be an optimization of sorts, but yes, it's turned into a bug or exposure of one of those unwritten/undocumented code paths which "expected" to have some other code path do the add. As for a different way to fix the problem... When a controller is really added into the domain it will gain an alias (which is what those error messages are complaining about). So, what if qemuDomainFindOrCreateSCSIDiskController checked the "found" cont in the first loop for "cont->info.alias". If it doesn't exist, then fall through under the "assumption" that the controller was added by some other mechanism into the list? That would require some more adjustments in qemuDomainAttachControllerDevice to know the controller was already in the list, but not active in the domain. John
On 12/02/2015 04:08 PM, John Ferlan wrote:
On 12/02/2015 03:27 AM, Boris Fiuczynski wrote:
On 12/02/2015 01:16 AM, John Ferlan wrote:
On 11/30/2015 06:05 AM, Boris Fiuczynski wrote:
[...]
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index cbfc41e..69cfd0f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -16285,6 +16285,9 @@ virDomainDefParseXML(xmlDocPtr xml, }
def->hostdevs[def->nhostdevs++] = hostdev; + + if (virDomainDefMaybeAddHostdevSCSIcontroller(def) < 0) + goto error; } VIR_FREE(nodes);
After some more experimentation and making sure my debug environment was "up to date" (e.g. had virtlogd started)...
Using top of tree I can hotplug a hostdev as long as it has a drive address defined. Not sure why it was failing before, but at least things are making more sense now. The reason why that path works with top of tree is because virDomainDeviceDefPostParseInternal won't call virDomainHostdevAssignAddress since hdev->info->type is not VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE. That means when the qemuDomainFindOrCreateSCSIDiskController is called it will do the magic add.
The path for this patch handles the inactive domain definition. While patch 2 handles the backend of the active domain path.
Patch 2 is also used in the "inactive domain definition" case. The important difference is that the controller is NOT created on the device definition path so that the controller is added via hotplug by qemuDomainFindOrCreateSCSIDiskController since device hotplug uses the device definition path! That is why this patch reintroduces adding the controller into domain definition that the hotplug code does not use.
When you have a running domain, then qemuDomainAttachDeviceFlags (e.g. what virsh attach-device would use) calls virDomainDeviceDefParse which is where the controller is added during virDomainDeviceDefPostParse. So patch 2 where you're removing that PostParse callback is why the hotplug will then work because no longer does post parse add the controller to the domain def.
Correct.
The call to qemuDomainFindOrCreateSCSIDiskController called by qemuDomainAttachSCSIDisk during qemuDomainAttachDeviceDiskLive in the live path of qemuDomainAttachDeviceFlags would then automagically add the controller. Since the active path wouldn't have added it during post parse.
Correct.
While these patches would work for the active domain hostdev add, I think perhaps a different mechanism should be used. Given the failure from trying to add a disk in the same environment, I'm not sure either patch solves the root problem that during hotplug we're relying on *not* adding a controller device when a new one is deemed necessary (for both hostdev and disk).
I think that the root problem was introduced by moving the adding of the controller from the DOMAIN definition parsing into the DEVICE definition parsing.
It's all domain_conf code...
virDomainHostdevDefParseXML is either called by virDomainDefParseXML (inactive) or virDomainDeviceDefParse (active, via various drivers).
The post parse processing code for driver callbacks is handled more directly through virDomainDeviceDefPostParse at the end of virDomainDeviceDefParse.
The post parse processing for inactive callbacks is handled through virDomainDefPostParseDeviceIterator as part of validating the domain in virDomainDefPostParse.
IMO it's splitting hairs to claim device processing is that much different than domain processing. It's just that the hotplug code knows how to directly go to the device it wants based on the xml from the attach-device command rather than going through all domain processing.
In any case, the existing mechanism to assign an address and thus maybe add a controller for the inactive domain was introduced by commit id 'cdb978955'. That also introduced the code used by the active/hotplug processing. The "side effect" of the code not trying to add a controller was that setting the controller value to something didn't exist (I assume) knowing that CreateSCSIDiskController would do the dirty work later (not documented either which is another "issue" in my mind).
Just for completeness... Auto-generating a controller for a hostdev was introduced by commit id '881eb780'. The CreateSCSIDiskController code has been around a lot longer...
Since the code at this point seems by its multiple usage patterns rather complex and fragile to changes I choose for this fix to revert the code rather than to reengineer the code and not potentially make things worse.
I think this may be a short term solution unless I can figure out a better way short term to hotplug an added controller as well if one is created.
One thing that is of interest is that virDomainDefMaybeAddController has 3 return values -1 fail, 0 didn't add a controller, and 1 added a controller. I'm wondering if that can somehow be used to ensure that if we add a controller that we ensure it really gets plugged in. Instead of relying on qemuDomainFindOrCreateSCSIDiskController to backdoor it in.
I must say that calling vir > DomainDef < MaybeAddController from the DEVICE parsing and not from the DOMAIN parsing feels rather strange to me. In both cases domain parsing and hotplugging devices the controllers are plugged in AFTER the device parsing is finished! I do not see any backdoor on the hotplug path!
The "concept" of moving virDomainDefMaybeAddController into virDomainHostdevAssignAddress was I believe meant to be an optimization of sorts, but yes, it's turned into a bug or exposure of one of those unwritten/undocumented code paths which "expected" to have some other code path do the add.
As for a different way to fix the problem... When a controller is really added into the domain it will gain an alias (which is what those error messages are complaining about).
So, what if qemuDomainFindOrCreateSCSIDiskController checked the "found" cont in the first loop for "cont->info.alias". If it doesn't exist, then fall through under the "assumption" that the controller was added by some other mechanism into the list? That would require some more adjustments in qemuDomainAttachControllerDevice to know the controller was already in the list, but not active in the domain.
I thought about that as well and decided against it because you would add controllers in the domain data structure and than decide based on the aliases if the controller needs to be hotplugged afterwards. I thought that using the alias to detect is the domain is running in such way would not be proper! Anyhow what are you going to do if hotplugging the controller fails? Would this require an additional cleanup method? qemuDomainAttachControllerDevice is also used when adding a new scsi controller directly from virsh attach-device (scsi-controller.xml). What additional special casing would be required there...?
John
-- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martina Köderitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On 12/02/2015 11:37 AM, Boris Fiuczynski wrote: [...]
So, what if qemuDomainFindOrCreateSCSIDiskController checked the "found" cont in the first loop for "cont->info.alias". If it doesn't exist, then fall through under the "assumption" that the controller was added by some other mechanism into the list? That would require some more adjustments in qemuDomainAttachControllerDevice to know the controller was already in the list, but not active in the domain.
I thought about that as well and decided against it because you would add controllers in the domain data structure and than decide based on the aliases if the controller needs to be hotplugged afterwards. I thought that using the alias to detect is the domain is running in such way would not be proper! Anyhow what are you going to do if hotplugging the controller fails? Would this require an additional cleanup method? qemuDomainAttachControllerDevice is also used when adding a new scsi controller directly from virsh attach-device (scsi-controller.xml). What additional special casing would be required there...?
I had just started down that path... not that I like that option, but it was an idea. I was able to get it to work in the one case, but other issues showed up, so I'll just abandon it... Although we're now in freeze, I will go with these two patches. I think it's better to combine them - mostly to make it "clearer" for anyone venturing down this path in the future to see the relationship(s). The combined commit message would be (the weird line breaks only occur here because of my settings on line width/break for sent emails): conf: Revert some code to resolve issues for hostdev hotplug This patch reverts parts of commits 0d8b24f6b and 0785966d dealing with the addition of a controller during virDomainHostdevAssignAddress. This caused a regression for the hostdev hotplug path which assumes the qemuDomainFindOrCreateSCSIDiskController will add the new controller during qemuDomainAttachHostSCSIDevice to both the running domain and the domain def controller list when the controller doesn't yet exist (whether due to no SCSI controllers existing or the addition of a new controller because existing ones are full). Since commit id 0d8b24f6 will call virDomainHostdevAssignAddress during virDomainDeviceDefPostParseInternal which is called either during domain definition post processing (via an iterator during virDomainDefPostParse) or directly from virDomainDeviceDefParse during hotplug, the change broke the "side effect" of being able to add both a hostdev and controller to the running domain. The regression would only be seen if the running domain didn't have a SCSI controller alrady defined or if the existing SCSI controller was "full" of devices and a new controller needed to be created. This patch will also add some extra comments to the code to avoid a similar future change. I'd like to also add a pair of comments to also leave a second trail of breadcrumbs... In virDomainHostdevAssignAddress after the initial loop: /* NB: Do not attempt calling virDomainDefMaybeAddController to * automagically add a "new" controller. Doing so will result in * qemuDomainFindOrCreateSCSIDiskController "finding" the controller * in the domain def list and thus not hotplugging the controller as * well as the hostdev in the event that there are either no SCSI * controllers defined or there was no space on an existing one. */ and in virDomainDefParseXML prior to maybe adding the controller: /* For a domain definition, we need to check if the controller * for this hostdev exists yet and if not add it. This cannot be * done during virDomainHostdevAssignAddress (as part of device * post processing) because that will result in the failure to * load the controller during hostdev hotplug. */ Does this all seem reasonable? John
On 12/02/2015 06:13 PM, John Ferlan wrote:
On 12/02/2015 11:37 AM, Boris Fiuczynski wrote:
[...]
So, what if qemuDomainFindOrCreateSCSIDiskController checked the "found" cont in the first loop for "cont->info.alias". If it doesn't exist, then fall through under the "assumption" that the controller was added by some other mechanism into the list? That would require some more adjustments in qemuDomainAttachControllerDevice to know the controller was already in the list, but not active in the domain.
I thought about that as well and decided against it because you would add controllers in the domain data structure and than decide based on the aliases if the controller needs to be hotplugged afterwards. I thought that using the alias to detect is the domain is running in such way would not be proper! Anyhow what are you going to do if hotplugging the controller fails? Would this require an additional cleanup method? qemuDomainAttachControllerDevice is also used when adding a new scsi controller directly from virsh attach-device (scsi-controller.xml). What additional special casing would be required there...?
I had just started down that path... not that I like that option, but it was an idea. I was able to get it to work in the one case, but other issues showed up, so I'll just abandon it...
Although we're now in freeze, I will go with these two patches. I think
I would like to encourage you to also pick the third patch fixing the SCSI disk hotplug scenario.
it's better to combine them - mostly to make it "clearer" for anyone venturing down this path in the future to see the relationship(s). Agreed!
The combined commit message would be (the weird line breaks only occur here because of my settings on line width/break for sent emails):
conf: Revert some code to resolve issues for hostdev hotplug
This patch reverts parts of commits 0d8b24f6b and 0785966d dealing with the addition of a controller during virDomainHostdevAssignAddress. This caused a regression for the hostdev hotplug path which assumes the qemuDomainFindOrCreateSCSIDiskController will add the new controller during qemuDomainAttachHostSCSIDevice to both the running domain and the domain def controller list when the controller doesn't yet exist (whether due to no SCSI controllers existing or the addition of a new controller because existing ones are full).
Since commit id 0d8b24f6 will call virDomainHostdevAssignAddress during virDomainDeviceDefPostParseInternal which is called either during domain definition post processing (via an iterator during virDomainDefPostParse) or directly from virDomainDeviceDefParse during hotplug, the change broke the "side effect" of being able to add both a hostdev and controller to the running domain.
The regression would only be seen if the running domain didn't have a SCSI controller alrady defined or if the existing SCSI controller was "full" of devices and a new controller needed to be created.
This patch will also add some extra comments to the code to avoid a similar future change.
Reads good.
I'd like to also add a pair of comments to also leave a second trail of breadcrumbs...
In virDomainHostdevAssignAddress after the initial loop:
/* NB: Do not attempt calling virDomainDefMaybeAddController to * automagically add a "new" controller. Doing so will result in * qemuDomainFindOrCreateSCSIDiskController "finding" the controller * in the domain def list and thus not hotplugging the controller as * well as the hostdev in the event that there are either no SCSI * controllers defined or there was no space on an existing one. */
and in virDomainDefParseXML prior to maybe adding the controller:
/* For a domain definition, we need to check if the controller * for this hostdev exists yet and if not add it. This cannot be * done during virDomainHostdevAssignAddress (as part of device * post processing) because that will result in the failure to * load the controller during hostdev hotplug. */
Does this all seem reasonable?
Yes, and (again) I would like to ask to also add the third patch as the second patch to fix the scsi disk hotplug problem. -- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martina Köderitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On 12/03/2015 02:05 PM, Boris Fiuczynski wrote:
conf: Revert some code to resolve issues for hostdev hotplug
This patch reverts parts of commits 0d8b24f6b and 0785966d dealing with the addition of a controller during virDomainHostdevAssignAddress. This caused a regression for the hostdev hotplug path which assumes the qemuDomainFindOrCreateSCSIDiskController will add the new controller during qemuDomainAttachHostSCSIDevice to both the running domain and the domain def controller list when the controller doesn't yet exist (whether due to no SCSI controllers existing or the addition of a new controller because existing ones are full).
Since commit id 0d8b24f6 will call virDomainHostdevAssignAddress during virDomainDeviceDefPostParseInternal which is called either during domain definition post processing (via an iterator during virDomainDefPostParse) or directly from virDomainDeviceDefParse during hotplug, the change broke the "side effect" of being able to add both a hostdev and controller to the running domain.
The regression would only be seen if the running domain didn't have a SCSI controller alrady defined or if the existing SCSI controller was typo already
"full" of devices and a new controller needed to be created.
This patch will also add some extra comments to the code to avoid a similar future change.
-- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martina Köderitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On 12/03/2015 08:20 AM, Boris Fiuczynski wrote:
On 12/03/2015 02:05 PM, Boris Fiuczynski wrote:
conf: Revert some code to resolve issues for hostdev hotplug
This patch reverts parts of commits 0d8b24f6b and 0785966d dealing with the addition of a controller during virDomainHostdevAssignAddress. This caused a regression for the hostdev hotplug path which assumes the qemuDomainFindOrCreateSCSIDiskController will add the new controller during qemuDomainAttachHostSCSIDevice to both the running domain and the domain def controller list when the controller doesn't yet exist (whether due to no SCSI controllers existing or the addition of a new controller because existing ones are full).
Since commit id 0d8b24f6 will call virDomainHostdevAssignAddress during virDomainDeviceDefPostParseInternal which is called either during domain definition post processing (via an iterator during virDomainDefPostParse) or directly from virDomainDeviceDefParse during hotplug, the change broke the "side effect" of being able to add both a hostdev and controller to the running domain.
The regression would only be seen if the running domain didn't have a SCSI controller alrady defined or if the existing SCSI controller was typo already
"full" of devices and a new controller needed to be created.
This patch will also add some extra comments to the code to avoid a similar future change.
I have pushed this pair of changes as one patch. I'm hesitant to push the third patch because I'm not sure what it might break. I'll address it in a response to that patch though... John
This reverts commit 0785966d03c9b26ce127aadb93b4d3130a3af79f. Trying to hotplug a scsi hostdev to a domain without the required SCSI controller fails. Commits 0d8b24f6 and 0785966d rearranged the automatic creation of the required SCSI controllers from the domain parsing xml code into the device post xml parsing code section. Doing so will create but not hotplug the missing SCSI controller and on the hotplug path through the same code the SCSI controller will not get hotplugged since it already exits. When the SCSI hostdev device afterwards is hotplugged the following internal error occurs: error: Failed to attach device from scsihostdev.xml error: internal error: Device alias was not set for scsi controller with index 0 This patch reverts the code from the device post xml parsing code section. Signed-off-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Reviewed-by: Stefan Zimmermann <stzi@linux.vnet.ibm.com> --- src/conf/domain_conf.c | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 69cfd0f..6fb70db 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4181,7 +4181,7 @@ virDomainHostdevAssignAddress(virDomainXMLOptionPtr xmlopt, int next_unit = 0; unsigned controller = 0; size_t i; - int ret = -1; + int ret; for (i = 0; i < def->ncontrollers; i++) { if (def->controllers[i]->type != VIR_DOMAIN_CONTROLLER_TYPE_SCSI) @@ -4200,17 +4200,6 @@ virDomainHostdevAssignAddress(virDomainXMLOptionPtr xmlopt, } } - /* If failed to find any VIR_DOMAIN_CONTROLLER_TYPE_SCSI or any space - * on existing VIR_DOMAIN_CONTROLLER_TYPE_SCSI controller(s), then - * try to add a new controller resulting in placement of this entry - * as unit=0 - */ - if (ret == -1 && - virDomainDefMaybeAddController((virDomainDefPtr) def, - VIR_DOMAIN_CONTROLLER_TYPE_SCSI, - controller, -1) < 0) - return -1; - hostdev->info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE; hostdev->info->addr.drive.controller = controller; hostdev->info->addr.drive.bus = 0; -- 2.3.0
When a SCSI disk is hotplugged to a domain that does not have the required scsi controller already defined the following internal error occurs error: Failed to attach device from scsi_disk.xml error: internal error: Could not find scsi controller with index 0 required for device Commit 0260506c added in method qemuBuildDriveDevStr a lookup of the controller alias. The internal error occurs because in method qemuDomainAttachSCSIDisk the automatic creation of the potentially missing SCSI controller occurs after calling qemuBuildDriveDevStr. This patch reverses the calling sequence. Signed-off-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Reviewed-by: Stefan Zimmermann <stzi@linux.vnet.ibm.com> --- src/qemu/qemu_hotplug.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 8804d3d..210d485 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -607,6 +607,12 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn, goto error; } + for (i = 0; i <= disk->info.addr.drive.controller; i++) { + cont = qemuDomainFindOrCreateSCSIDiskController(driver, vm, i); + if (!cont) + goto error; + } + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { if (qemuAssignDeviceDiskAlias(vm->def, disk, priv->qemuCaps) < 0) goto error; @@ -617,12 +623,6 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn, if (!(drivestr = qemuBuildDriveStr(conn, disk, false, priv->qemuCaps))) goto error; - for (i = 0; i <= disk->info.addr.drive.controller; i++) { - cont = qemuDomainFindOrCreateSCSIDiskController(driver, vm, i); - if (!cont) - goto error; - } - /* Tell clang that "cont" is non-NULL. This is because disk->info.addr.driver.controller is unsigned, and hence the above loop must iterate at least once. */ -- 2.3.0
On 11/30/2015 06:06 AM, Boris Fiuczynski wrote:
When a SCSI disk is hotplugged to a domain that does not have the required scsi controller already defined the following internal error occurs
error: Failed to attach device from scsi_disk.xml error: internal error: Could not find scsi controller with index 0 required for device
Commit 0260506c added in method qemuBuildDriveDevStr a lookup of the controller alias. The internal error occurs because in method qemuDomainAttachSCSIDisk the automatic creation of the potentially missing SCSI controller occurs after calling qemuBuildDriveDevStr.
This patch reverses the calling sequence.
Signed-off-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Reviewed-by: Stefan Zimmermann <stzi@linux.vnet.ibm.com> --- src/qemu/qemu_hotplug.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 8804d3d..210d485 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -607,6 +607,12 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn, goto error; }
Would you say the following is an accurate description to add? /* Let's make sure our disk has a controller defined and loaded * before trying add the disk. The virDomainDiskDefAssignAddress * doesn't try to add a controller if one doesn't exist, it just * assigns the disk to the calculated spot. */
+ for (i = 0; i <= disk->info.addr.drive.controller; i++) { + cont = qemuDomainFindOrCreateSCSIDiskController(driver, vm, i); + if (!cont) + goto error; + } + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { if (qemuAssignDeviceDiskAlias(vm->def, disk, priv->qemuCaps) < 0) goto error; @@ -617,12 +623,6 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn, if (!(drivestr = qemuBuildDriveStr(conn, disk, false, priv->qemuCaps))) goto error;
- for (i = 0; i <= disk->info.addr.drive.controller; i++) { - cont = qemuDomainFindOrCreateSCSIDiskController(driver, vm, i); - if (!cont) - goto error; - } - /* Tell clang that "cont" is non-NULL. This is because disk->info.addr.driver.controller is unsigned, and hence the above loop must iterate at least once. */
Is there a reason you chose to not grab the clang check too? John
On 12/03/2015 05:56 PM, John Ferlan wrote:
On 11/30/2015 06:06 AM, Boris Fiuczynski wrote:
When a SCSI disk is hotplugged to a domain that does not have the required scsi controller already defined the following internal error occurs
error: Failed to attach device from scsi_disk.xml error: internal error: Could not find scsi controller with index 0 required for device
Commit 0260506c added in method qemuBuildDriveDevStr a lookup of the controller alias. The internal error occurs because in method qemuDomainAttachSCSIDisk the automatic creation of the potentially missing SCSI controller occurs after calling qemuBuildDriveDevStr.
This patch reverses the calling sequence.
Signed-off-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Reviewed-by: Stefan Zimmermann <stzi@linux.vnet.ibm.com> --- src/qemu/qemu_hotplug.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 8804d3d..210d485 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -607,6 +607,12 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn, goto error; }
Would you say the following is an accurate description to add?
/* Let's make sure our disk has a controller defined and loaded * before trying add the disk. The virDomainDiskDefAssignAddress * doesn't try to add a controller if one doesn't exist, it just * assigns the disk to the calculated spot. */
First sentence I can agree to. The second sentence I am not sure I understand. Did you mean: The controller the disk is going to use must exist before a qemu command line string is being generated.
+ for (i = 0; i <= disk->info.addr.drive.controller; i++) { + cont = qemuDomainFindOrCreateSCSIDiskController(driver, vm, i); + if (!cont) + goto error; + } + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { if (qemuAssignDeviceDiskAlias(vm->def, disk, priv->qemuCaps) < 0) goto error; @@ -617,12 +623,6 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn, if (!(drivestr = qemuBuildDriveStr(conn, disk, false, priv->qemuCaps))) goto error;
- for (i = 0; i <= disk->info.addr.drive.controller; i++) { - cont = qemuDomainFindOrCreateSCSIDiskController(driver, vm, i); - if (!cont) - goto error; - } - /* Tell clang that "cont" is non-NULL. This is because disk->info.addr.driver.controller is unsigned, and hence the above loop must iterate at least once. */
Is there a reason you chose to not grab the clang check too?
yes, it's a miss... good catch! ;-)
John
-- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martina Köderitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On 12/04/2015 12:07 PM, Boris Fiuczynski wrote:
On 12/03/2015 05:56 PM, John Ferlan wrote:
On 11/30/2015 06:06 AM, Boris Fiuczynski wrote:
When a SCSI disk is hotplugged to a domain that does not have the required scsi controller already defined the following internal error occurs
error: Failed to attach device from scsi_disk.xml error: internal error: Could not find scsi controller with index 0 required for device
Commit 0260506c added in method qemuBuildDriveDevStr a lookup of the controller alias. The internal error occurs because in method qemuDomainAttachSCSIDisk the automatic creation of the potentially missing SCSI controller occurs after calling qemuBuildDriveDevStr.
This patch reverses the calling sequence.
Signed-off-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Reviewed-by: Stefan Zimmermann <stzi@linux.vnet.ibm.com> --- src/qemu/qemu_hotplug.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 8804d3d..210d485 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -607,6 +607,12 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn, goto error; }
Would you say the following is an accurate description to add?
/* Let's make sure our disk has a controller defined and loaded * before trying add the disk. The virDomainDiskDefAssignAddress * doesn't try to add a controller if one doesn't exist, it just * assigns the disk to the calculated spot. */
First sentence I can agree to. The second sentence I am not sure I understand. Did you mean: The controller the disk is going to use must exist before a qemu command line string is being generated.
It's meant to point out the place in the code where the address could have been generated; however, I suppose a user could have provided an address and that doesn't make sense. I used your suggestion and pushed... Thanks for finding and resolving this... John
+ for (i = 0; i <= disk->info.addr.drive.controller; i++) { + cont = qemuDomainFindOrCreateSCSIDiskController(driver, vm, i); + if (!cont) + goto error; + } + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { if (qemuAssignDeviceDiskAlias(vm->def, disk, priv->qemuCaps) < 0) goto error; @@ -617,12 +623,6 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn, if (!(drivestr = qemuBuildDriveStr(conn, disk, false, priv->qemuCaps))) goto error;
- for (i = 0; i <= disk->info.addr.drive.controller; i++) { - cont = qemuDomainFindOrCreateSCSIDiskController(driver, vm, i); - if (!cont) - goto error; - } - /* Tell clang that "cont" is non-NULL. This is because disk->info.addr.driver.controller is unsigned, and hence the above loop must iterate at least once. */
Is there a reason you chose to not grab the clang check too?
yes, it's a miss... good catch! ;-)
John
participants (2)
-
Boris Fiuczynski -
John Ferlan