[libvirt] [PATCH 0/4] Resolve issue with duplicated SCSI hostdev addresses
Details in the various patches. John Ferlan (4): qemu: Tolerate storage source private data being NULL for hotplug SCSI hostdev qemu: Use same model when adding hostdev SCSI controller conf: Use existing SCSI hostdev model to create new conf: Fix generating addresses for SCSI hostdev src/conf/domain_conf.c | 44 ++++++++++++---------- src/qemu/qemu_hotplug.c | 19 +++++++--- .../hostdev-scsi-autogen-address.xml | 2 +- 3 files changed, 39 insertions(+), 26 deletions(-) -- 2.13.6
Commit id 'c5c96545' neglected to validate that the srcPriv was non-NULL before dereferencing. Similar problem to what was fixed by commit id '8056721c' but missed during multiple rebases and code reworks. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_hotplug.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 6ef28bf05..9317e134a 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2286,7 +2286,7 @@ qemuDomainAttachHostSCSIDevice(virConnectPtr conn, virDomainHostdevSubsysSCSIPtr scsisrc = &hostdev->source.subsys.u.scsi; virDomainHostdevSubsysSCSIiSCSIPtr iscsisrc = &scsisrc->u.iscsi; qemuDomainStorageSourcePrivatePtr srcPriv; - qemuDomainSecretInfoPtr secinfo; + qemuDomainSecretInfoPtr secinfo = NULL; if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE_SCSI_GENERIC)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", @@ -2328,7 +2328,8 @@ qemuDomainAttachHostSCSIDevice(virConnectPtr conn, goto cleanup; srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(iscsisrc->src); - secinfo = srcPriv->secinfo; + if (srcPriv) + secinfo = srcPriv->secinfo; if (secinfo && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES) { if (qemuBuildSecretInfoProps(secinfo, &secobjProps) < 0) goto cleanup; -- 2.13.6
On 12/06/2017 08:08 AM, John Ferlan wrote:
Commit id 'c5c96545' neglected to validate that the srcPriv was non-NULL before dereferencing. Similar problem to what was fixed by commit id '8056721c' but missed during multiple rebases and code reworks.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Eric Farman <farman@linux.vnet.ibm.com>
--- src/qemu/qemu_hotplug.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 6ef28bf05..9317e134a 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2286,7 +2286,7 @@ qemuDomainAttachHostSCSIDevice(virConnectPtr conn, virDomainHostdevSubsysSCSIPtr scsisrc = &hostdev->source.subsys.u.scsi; virDomainHostdevSubsysSCSIiSCSIPtr iscsisrc = &scsisrc->u.iscsi; qemuDomainStorageSourcePrivatePtr srcPriv; - qemuDomainSecretInfoPtr secinfo; + qemuDomainSecretInfoPtr secinfo = NULL;
if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE_SCSI_GENERIC)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", @@ -2328,7 +2328,8 @@ qemuDomainAttachHostSCSIDevice(virConnectPtr conn, goto cleanup;
srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(iscsisrc->src); - secinfo = srcPriv->secinfo; + if (srcPriv) + secinfo = srcPriv->secinfo; if (secinfo && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES) { if (qemuBuildSecretInfoProps(secinfo, &secobjProps) < 0) goto cleanup;
On 12/13/2017 10:06 AM, Eric Farman wrote:
On 12/06/2017 08:08 AM, John Ferlan wrote:
Commit id 'c5c96545' neglected to validate that the srcPriv was non-NULL before dereferencing. Similar problem to what was fixed by commit id '8056721c' but missed during multiple rebases and code reworks.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Eric Farman <farman@linux.vnet.ibm.com>
Thanks - I went ahead and pushed this while the rest is being worked out. Tks - John
When qemuDomainFindOrCreateSCSIDiskController adds a controller, let's use the same model as a currently found controller under the assumption that the reason to add the controller in hotplug is because virDomainHostdevAssignAddress determined that there were too many devices on the existing controller, but only assigned a new controller index and did not add a new controller and we desire to use the same controller model as any existing conroller and not take a chance that qemuDomainSetSCSIControllerModel would use a default that may be incompatible. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_hotplug.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 9317e134a..90d50e7b1 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -587,6 +587,7 @@ qemuDomainFindOrCreateSCSIDiskController(virQEMUDriverPtr driver, { size_t i; virDomainControllerDefPtr cont; + virDomainControllerModelSCSI model = -1; for (i = 0; i < vm->def->ncontrollers; i++) { cont = vm->def->controllers[i]; @@ -596,6 +597,12 @@ qemuDomainFindOrCreateSCSIDiskController(virQEMUDriverPtr driver, if (cont->idx == controller) return cont; + + /* Save off the model - if we end up creating a controller it's + * because the user didn't provide one and we need to automagically + * create one because the existing one is full - so let's be sure + * to keep the same model in that case. */ + model = cont->model; } /* No SCSI controller present, for backward compatibility we @@ -604,11 +611,10 @@ qemuDomainFindOrCreateSCSIDiskController(virQEMUDriverPtr driver, return NULL; cont->type = VIR_DOMAIN_CONTROLLER_TYPE_SCSI; cont->idx = controller; - cont->model = -1; + cont->model = model; - VIR_INFO("No SCSI controller present, hotplugging one"); - if (qemuDomainAttachControllerDevice(driver, - vm, cont) < 0) { + VIR_INFO("No SCSI controller present, hotplugging one model=%d", model); + if (qemuDomainAttachControllerDevice(driver, vm, cont) < 0) { VIR_FREE(cont); return NULL; } -- 2.13.6
On 12/06/2017 08:08 AM, John Ferlan wrote:
When qemuDomainFindOrCreateSCSIDiskController adds a controller, let's use the same model as a currently found controller under the assumption that the reason to add the controller in hotplug is because virDomainHostdevAssignAddress determined that there were too many devices on the existing controller, but only assigned a new controller index and did not add a new controller and we desire to use the same controller model as any existing conroller
s/conroller/controller
and not take a chance that qemuDomainSetSCSIControllerModel would use a default that may be incompatible.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_hotplug.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 9317e134a..90d50e7b1 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -587,6 +587,7 @@ qemuDomainFindOrCreateSCSIDiskController(virQEMUDriverPtr driver, { size_t i; virDomainControllerDefPtr cont; + virDomainControllerModelSCSI model = -1;
for (i = 0; i < vm->def->ncontrollers; i++) { cont = vm->def->controllers[i]; @@ -596,6 +597,12 @@ qemuDomainFindOrCreateSCSIDiskController(virQEMUDriverPtr driver,
if (cont->idx == controller) return cont; + + /* Save off the model - if we end up creating a controller it's + * because the user didn't provide one and we need to automagically + * create one because the existing one is full - so let's be sure + * to keep the same model in that case. */ + model = cont->model;
Hrm... I'm missing something... This is confusing, because nothing in this loop tells us the controller we find is full, just that we have a TYPE_SCSI controller and the index isn't found. And if the index WAS found, we would've exited before this line anyway so we don't know if it was full or not. Nothing in a <hostdev> tag says what type of controller we want, so what happens when controller[1] is virtio-scsi, and controller[2] is LSI? This will create a second LSI controller which wouldn't help if we're hotplugging another virtio-scsi device and controller[1] was full. (Is mixing SCSI controller types common? I don't know, I stay in a little virtio-scsi playpen.) And I'm still trying to figure out how qemuDomainSetSCSIControllerModel plays into this and other codepaths. Sorry, just sending my questions because this code is more fresh in your mind than my own.
}
/* No SCSI controller present, for backward compatibility we @@ -604,11 +611,10 @@ qemuDomainFindOrCreateSCSIDiskController(virQEMUDriverPtr driver, return NULL; cont->type = VIR_DOMAIN_CONTROLLER_TYPE_SCSI; cont->idx = controller; - cont->model = -1; + cont->model = model;
- VIR_INFO("No SCSI controller present, hotplugging one"); - if (qemuDomainAttachControllerDevice(driver, - vm, cont) < 0) { + VIR_INFO("No SCSI controller present, hotplugging one model=%d", model);
Seems like a good use for virDomainControllerModelSCSITypeToString(model) perhaps?
+ if (qemuDomainAttachControllerDevice(driver, vm, cont) < 0) { VIR_FREE(cont); return NULL; }
On 12/13/2017 10:43 AM, Eric Farman wrote:
On 12/06/2017 08:08 AM, John Ferlan wrote:
When qemuDomainFindOrCreateSCSIDiskController adds a controller, let's use the same model as a currently found controller under the assumption that the reason to add the controller in hotplug is because virDomainHostdevAssignAddress determined that there were too many devices on the existing controller, but only assigned a new controller index and did not add a new controller and we desire to use the same controller model as any existing conroller
s/conroller/controller
and not take a chance that qemuDomainSetSCSIControllerModel would use a default that may be incompatible.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_hotplug.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 9317e134a..90d50e7b1 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -587,6 +587,7 @@ qemuDomainFindOrCreateSCSIDiskController(virQEMUDriverPtr driver, { size_t i; virDomainControllerDefPtr cont; + virDomainControllerModelSCSI model = -1;
for (i = 0; i < vm->def->ncontrollers; i++) { cont = vm->def->controllers[i]; @@ -596,6 +597,12 @@ qemuDomainFindOrCreateSCSIDiskController(virQEMUDriverPtr driver,
if (cont->idx == controller) return cont; + + /* Save off the model - if we end up creating a controller it's + * because the user didn't provide one and we need to automagically + * create one because the existing one is full - so let's be sure + * to keep the same model in that case. */ + model = cont->model;
Hrm... I'm missing something...
This is confusing, because nothing in this loop tells us the controller we find is full, just that we have a TYPE_SCSI controller and the index isn't found. And if the index WAS found, we would've exited before this line anyway so we don't know if it was full or not.
Does this explanation help: /* Because virDomainHostdevAssignAddress called during * virDomainHostdevDefPostParse cannot add a new controller * it will assign a controller index to a controller that doesn't * exist leaving this code to perform the magic of adding the * controller. Because that code would be attempting to add a * SCSI disk to an existing controller, let's save off the model * of the "last" SCSI controller we find so that if we end up * creating a controller below it uses the same controller model. */ The magic key is in patch 4 where it's noted that for virDomainHostdevAssignAddress we need to assign a controller index to an <address> element where the controller itself doesn't yet exist.
Nothing in a <hostdev> tag says what type of controller we want, so what happens when controller[1] is virtio-scsi, and controller[2] is LSI? This will create a second LSI controller which wouldn't help if we're hotplugging another virtio-scsi device and controller[1] was full. (Is mixing SCSI controller types common? I don't know, I stay in a little virtio-scsi playpen.)
Yes, true, and that I think ends up being the perhaps unavoidable tragic flaw because after all how do you really know which model to use? I'm not opposed to adding that truth to the comment, but wording it could be a bit tricky... Ironically I do have patches that would "prefer" virtio-scsi for hostdev paths that I considered adding to this series, but wasn't sure it'd be accepted. Something that I'm reconsidering now that I've read Michal's patch on qemu-devel: http://lists.nongnu.org/archive/html/qemu-devel/2017-12/msg01125.html I also have another patch that would prefer virtio-scsi as the default for new SCSI controllers by swapping the QEMU_CAPS_VIRTIO_SCSI and QEMU_CAPS_SCSI_LSI if logic in qemuDomainSetSCSIControllerModel in the } else { condition. That way we'd choose virtio-scsi over lsi if it exists (at least for non pseries). That conceivably would make this patch unnecessary other than this patch would force the model value for the controller to be filled in rather than it being empty (e.g. -1) and then allowing the default code to take over. That's a different can of worms, but the answer to that is in one of your follow-up questions. Of course for the consumer that's being smart ;-) they would create enough virtio-scsi controllers for the number of SCSI LUN's they are adding and none of this matters.
And I'm still trying to figure out how qemuDomainSetSCSIControllerModel plays into this and other codepaths.
It's awful... If a controller model isn't provided, then we need to determine some value so that we create the correct command line. The code doesn't really change or set the default in the domain XML, just in the temporary @model variable used to generate the command line. Ironically I have another series posted regarding moving controller validation out of command line processing and into domain def validation processing. One of the patches there, Jan I believe asked why not set the model in post parse processing. Quite honestly I'm not sure, but I'm making a supposition based on a few years working on the project and that historically changing defaults or attempting to set some sort of policy has been a bit of an uphill battle. So for this series, I just took the path of least resistance. Now, I'm not opposed to setting the model in PostParse processing as long as we set it "smartly", but how do you do that without knowledge of the consumers intentions (or perhaps lack of knowledge). If we set during post parse processing, then we've set model policy going forward for the future for the domain since we'll save the value in the permanent/config file. This is opposed to perhaps allowing the future be able to "choose" the default that is the "most recent" and perhaps a better selection. Once we set/save it though - we're stuck with it. I'm not going to make *that* decision alone!
Sorry, just sending my questions because this code is more fresh in your mind than my own.
Good thing you asked this close to working on the code; my short term memory seems to fade faster each day ;-) And sorry for the long winded response - it's a be careful for what you ask!
}
/* No SCSI controller present, for backward compatibility we @@ -604,11 +611,10 @@ qemuDomainFindOrCreateSCSIDiskController(virQEMUDriverPtr driver, return NULL; cont->type = VIR_DOMAIN_CONTROLLER_TYPE_SCSI; cont->idx = controller; - cont->model = -1; + cont->model = model;
- VIR_INFO("No SCSI controller present, hotplugging one"); - if (qemuDomainAttachControllerDevice(driver, - vm, cont) < 0) { + VIR_INFO("No SCSI controller present, hotplugging one model=%d", model);
Seems like a good use for virDomainControllerModelSCSITypeToString(model) perhaps?
That could be done too... Tks - John
+ if (qemuDomainAttachControllerDevice(driver, vm, cont) < 0) { VIR_FREE(cont); return NULL; }
On 12/13/2017 02:39 PM, John Ferlan wrote:
On 12/13/2017 10:43 AM, Eric Farman wrote:
On 12/06/2017 08:08 AM, John Ferlan wrote:
When qemuDomainFindOrCreateSCSIDiskController adds a controller, let's use the same model as a currently found controller under the assumption that the reason to add the controller in hotplug is because virDomainHostdevAssignAddress determined that there were too many devices on the existing controller, but only assigned a new controller index and did not add a new controller and we desire to use the same controller model as any existing conroller
s/conroller/controller
and not take a chance that qemuDomainSetSCSIControllerModel would use a default that may be incompatible.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_hotplug.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 9317e134a..90d50e7b1 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -587,6 +587,7 @@ qemuDomainFindOrCreateSCSIDiskController(virQEMUDriverPtr driver, { size_t i; virDomainControllerDefPtr cont; + virDomainControllerModelSCSI model = -1;
for (i = 0; i < vm->def->ncontrollers; i++) { cont = vm->def->controllers[i]; @@ -596,6 +597,12 @@ qemuDomainFindOrCreateSCSIDiskController(virQEMUDriverPtr driver,
if (cont->idx == controller) return cont; + + /* Save off the model - if we end up creating a controller it's + * because the user didn't provide one and we need to automagically + * create one because the existing one is full - so let's be sure + * to keep the same model in that case. */ + model = cont->model;
Hrm... I'm missing something...
This is confusing, because nothing in this loop tells us the controller we find is full, just that we have a TYPE_SCSI controller and the index isn't found. And if the index WAS found, we would've exited before this line anyway so we don't know if it was full or not.
Does this explanation help:
/* Because virDomainHostdevAssignAddress called during * virDomainHostdevDefPostParse cannot add a new controller * it will assign a controller index to a controller that doesn't * exist leaving this code to perform the magic of adding the * controller. Because that code would be attempting to add a * SCSI disk to an existing controller, let's save off the model * of the "last" SCSI controller we find so that if we end up * creating a controller below it uses the same controller model. */
Yeah, that helps considerably.
The magic key is in patch 4 where it's noted that for virDomainHostdevAssignAddress we need to assign a controller index to an <address> element where the controller itself doesn't yet exist.
I never got that far. :( But with the above clarification, consider this patch Reviewed-by: Eric Farman <farman@linux.vnet.ibm.com>
Nothing in a <hostdev> tag says what type of controller we want, so what happens when controller[1] is virtio-scsi, and controller[2] is LSI? This will create a second LSI controller which wouldn't help if we're hotplugging another virtio-scsi device and controller[1] was full. (Is mixing SCSI controller types common? I don't know, I stay in a little virtio-scsi playpen.)
Yes, true, and that I think ends up being the perhaps unavoidable tragic flaw because after all how do you really know which model to use? I'm not opposed to adding that truth to the comment, but wording it could be a bit tricky...
Ironically I do have patches that would "prefer" virtio-scsi for hostdev paths that I considered adding to this series, but wasn't sure it'd be accepted. Something that I'm reconsidering now that I've read Michal's patch on qemu-devel:
http://lists.nongnu.org/archive/html/qemu-devel/2017-12/msg01125.html
I also have another patch that would prefer virtio-scsi as the default for new SCSI controllers by swapping the QEMU_CAPS_VIRTIO_SCSI and QEMU_CAPS_SCSI_LSI if logic in qemuDomainSetSCSIControllerModel in the } else { condition. That way we'd choose virtio-scsi over lsi if it exists (at least for non pseries).
That is certainly interesting, though I get worried when defaults change since I don't understand the other options. In my environment, only "auto" and "virtio-scsi" will boot as type=scsi controllers. Everything else gives me some variation of an invalid configuration message. But that doesn't mean that's the case for other setups.
That conceivably would make this patch unnecessary other than this patch would force the model value for the controller to be filled in rather than it being empty (e.g. -1) and then allowing the default code to take over. That's a different can of worms, but the answer to that is in one of your follow-up questions.
Of course for the consumer that's being smart ;-) they would create enough virtio-scsi controllers for the number of SCSI LUN's they are adding and none of this matters.
Oh my guest configs ALWAYS have a controller tag. Definitely. I totally never forget to add one. :-)
And I'm still trying to figure out how qemuDomainSetSCSIControllerModel plays into this and other codepaths.
It's awful... If a controller model isn't provided, then we need to determine some value so that we create the correct command line. The code doesn't really change or set the default in the domain XML, just in the temporary @model variable used to generate the command line.
Ironically I have another series posted regarding moving controller validation out of command line processing and into domain def validation processing. One of the patches there, Jan I believe asked why not set the model in post parse processing. Quite honestly I'm not sure, but I'm making a supposition based on a few years working on the project and that historically changing defaults or attempting to set some sort of policy has been a bit of an uphill battle. So for this series, I just took the path of least resistance.
Now, I'm not opposed to setting the model in PostParse processing as long as we set it "smartly", but how do you do that without knowledge of the consumers intentions (or perhaps lack of knowledge).
Agreed. I can foresee host devices going to wrong controllers because of copy/paste errors, so trying to provide educated guesses in a scenario where the controller is auto-generated would be equally tough.
If we set during post parse processing, then we've set model policy going forward for the future for the domain since we'll save the value in the permanent/config file. This is opposed to perhaps allowing the future be able to "choose" the default that is the "most recent" and perhaps a better selection. Once we set/save it though - we're stuck with it. I'm not going to make *that* decision alone!
Sorry, just sending my questions because this code is more fresh in your mind than my own.
Good thing you asked this close to working on the code; my short term memory seems to fade faster each day ;-)
And sorry for the long winded response - it's a be careful for what you ask!
No, I appreciate the insight! I saw your vhost-scsi series this morning and thought I'd take that opportunity. I didn't see this series until your ping, so opted to follow this trail for a while too. Will pick up on other patches later methinks. - Eric
}
/* No SCSI controller present, for backward compatibility we @@ -604,11 +611,10 @@ qemuDomainFindOrCreateSCSIDiskController(virQEMUDriverPtr driver, return NULL; cont->type = VIR_DOMAIN_CONTROLLER_TYPE_SCSI; cont->idx = controller; - cont->model = -1; + cont->model = model;
- VIR_INFO("No SCSI controller present, hotplugging one"); - if (qemuDomainAttachControllerDevice(driver, - vm, cont) < 0) { + VIR_INFO("No SCSI controller present, hotplugging one model=%d", model);
Seems like a good use for virDomainControllerModelSCSITypeToString(model) perhaps?
That could be done too...
Tks -
John
+ if (qemuDomainAttachControllerDevice(driver, vm, cont) < 0) { VIR_FREE(cont); return NULL; }
On Wed, Dec 06, 2017 at 08:08:04AM -0500, John Ferlan wrote:
When qemuDomainFindOrCreateSCSIDiskController adds a controller, let's use the same model as a currently found controller under the assumption that the reason to add the controller in hotplug is because virDomainHostdevAssignAddress determined that there were too many devices on the existing controller, but only assigned a new controller index and did not add a new controller and we desire to use the same controller model as any existing conroller
s/conroller/controller/
and not take a chance that qemuDomainSetSCSIControllerModel would use a default that may be incompatible.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_hotplug.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)
ACK I like that this reduces the chances of model -1 appearing in XML, (maybe a step closer to removing qemuDomainSetSCSIControllerModel?). On the other hand, it does change the 'policy' of choosing the controller model. We don't plug one on PCI hotplug.
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 9317e134a..90d50e7b1 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c
[...]
@@ -596,6 +597,12 @@ qemuDomainFindOrCreateSCSIDiskController(virQEMUDriverPtr driver,
if (cont->idx == controller) return cont; + + /* Save off the model - if we end up creating a controller it's
What does 'save off' mean? Jan
+ * because the user didn't provide one and we need to automagically + * create one because the existing one is full - so let's be sure + * to keep the same model in that case. */ + model = cont->model; }
/* No SCSI controller present, for backward compatibility we
On 12/20/2017 07:46 AM, Ján Tomko wrote:
On Wed, Dec 06, 2017 at 08:08:04AM -0500, John Ferlan wrote:
When qemuDomainFindOrCreateSCSIDiskController adds a controller, let's use the same model as a currently found controller under the assumption that the reason to add the controller in hotplug is because virDomainHostdevAssignAddress determined that there were too many devices on the existing controller, but only assigned a new controller index and did not add a new controller and we desire to use the same controller model as any existing conroller
s/conroller/controller/
and not take a chance that qemuDomainSetSCSIControllerModel would use a default that may be incompatible.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_hotplug.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)
ACK
I like that this reduces the chances of model -1 appearing in XML, (maybe a step closer to removing qemuDomainSetSCSIControllerModel?). On the other hand, it does change the 'policy' of choosing the controller model. We don't plug one on PCI hotplug.
True... OTOH for this path, we're processing *just* a <hostdev> or <disk> and will be creating the controller hopefully based on what is existing and already full. I would hope that wouldn't be considered bad policy! I found more problems when having a full virtio-scsi controller and creating an LSI controller because that's the default. Beyond that, Michal noted something on qemu-devel about having more than one LUN on an LSI controller being a problem: http://lists.nongnu.org/archive/html/qemu-devel/2017-12/msg01125.html There's also some patches on qemu-devel where iSCSI LUN's on an LSI controller were causing crashes (perhaps something I ran into, but didn't spend the time debugging): http://lists.nongnu.org/archive/html/qemu-devel/2017-12/msg03119.html
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 9317e134a..90d50e7b1 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c
[...]
@@ -596,6 +597,12 @@ qemuDomainFindOrCreateSCSIDiskController(virQEMUDriverPtr driver,
if (cont->idx == controller) return cont; + + /* Save off the model - if we end up creating a controller it's
What does 'save off' mean?
Just an expression... For the most recent text I used see: https://www.redhat.com/archives/libvir-list/2017-December/msg00483.html I can still remove the "off" part John
Jan
+ * because the user didn't provide one and we need to automagically + * create one because the existing one is full - so let's be sure + * to keep the same model in that case. */ + model = cont->model; }
/* No SCSI controller present, for backward compatibility we
In virDomainDefMaybeAddHostdevSCSIcontroller when we add a new controller because someone neglected to add one or we're adding one because the existing one is full, we should copy over the model number from the existing controller since whatever we create should at least have the same characteristics as the one we cannot use because it's full. NB: This affects the existing hostdev-scsi-autogen-address test which would add a default ('lsi') SCSI controller for the various scsi_host's that would create a controller for the hostdev. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 13 ++++++++++++- tests/qemuxml2xmloutdata/hostdev-scsi-autogen-address.xml | 2 +- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 66e21c4bd..61b4a0075 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -17689,12 +17689,22 @@ virDomainDefMaybeAddHostdevSCSIcontroller(virDomainDefPtr def) size_t i; int maxController = -1; virDomainHostdevDefPtr hostdev; + virDomainControllerModelSCSI model = -1; + virDomainControllerModelSCSI newModel = -1; for (i = 0; i < def->nhostdevs; i++) { hostdev = def->hostdevs[i]; if (virHostdevIsSCSIDevice(hostdev) && (int)hostdev->info->addr.drive.controller > maxController) { maxController = hostdev->info->addr.drive.controller; + /* We may be creating a new controller because this one is full. + * So let's grab the model from it and update the model we're + * going to add as long as this one isn't undefined. The premise + * being keeping the same controller model for all SCSI hostdevs. */ + model = virDomainDeviceFindControllerModel(def, hostdev->info, + VIR_DOMAIN_CONTROLLER_TYPE_SCSI); + if (model != -1) + newModel = model; } } @@ -17702,7 +17712,8 @@ virDomainDefMaybeAddHostdevSCSIcontroller(virDomainDefPtr def) return 0; for (i = 0; i <= maxController; i++) { - if (virDomainDefMaybeAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_SCSI, i, -1) < 0) + if (virDomainDefMaybeAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_SCSI, + i, newModel) < 0) return -1; } diff --git a/tests/qemuxml2xmloutdata/hostdev-scsi-autogen-address.xml b/tests/qemuxml2xmloutdata/hostdev-scsi-autogen-address.xml index 8e93056ee..cea212b64 100644 --- a/tests/qemuxml2xmloutdata/hostdev-scsi-autogen-address.xml +++ b/tests/qemuxml2xmloutdata/hostdev-scsi-autogen-address.xml @@ -29,7 +29,7 @@ <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> </controller> <controller type='pci' index='0' model='pci-root'/> - <controller type='scsi' index='1'> + <controller type='scsi' index='1' model='virtio-scsi'> <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> </controller> <input type='mouse' bus='ps2'/> -- 2.13.6
On 12/06/2017 08:08 AM, John Ferlan wrote:
In virDomainDefMaybeAddHostdevSCSIcontroller when we add a new controller because someone neglected to add one or we're adding one because the existing one is full, we should copy over the model number from the existing controller since whatever we create should at least have the same characteristics as the one we cannot use because it's full.
NB: This affects the existing hostdev-scsi-autogen-address test which would add a default ('lsi') SCSI controller for the various scsi_host's that would create a controller for the hostdev.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 13 ++++++++++++- tests/qemuxml2xmloutdata/hostdev-scsi-autogen-address.xml | 2 +- 2 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 66e21c4bd..61b4a0075 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -17689,12 +17689,22 @@ virDomainDefMaybeAddHostdevSCSIcontroller(virDomainDefPtr def) size_t i; int maxController = -1; virDomainHostdevDefPtr hostdev; + virDomainControllerModelSCSI model = -1; + virDomainControllerModelSCSI newModel = -1;
for (i = 0; i < def->nhostdevs; i++) { hostdev = def->hostdevs[i]; if (virHostdevIsSCSIDevice(hostdev) && (int)hostdev->info->addr.drive.controller > maxController) { maxController = hostdev->info->addr.drive.controller; + /* We may be creating a new controller because this one is full. + * So let's grab the model from it and update the model we're + * going to add as long as this one isn't undefined. The premise + * being keeping the same controller model for all SCSI hostdevs. */ + model = virDomainDeviceFindControllerModel(def, hostdev->info, + VIR_DOMAIN_CONTROLLER_TYPE_SCSI); + if (model != -1) + newModel = model;
What's the harm if the last controller were undefined? Wouldn't it get populated down the road anyway if one were not set at this point (we send -1 today, after all). That could eliminate one of the two virDomainControllerModelSCSI variables here, I would think. But, either way I think it's fine. Reviewed-by: Eric Farman <farman@linux.vnet.ibm.com>
} }
@@ -17702,7 +17712,8 @@ virDomainDefMaybeAddHostdevSCSIcontroller(virDomainDefPtr def) return 0;
for (i = 0; i <= maxController; i++) { - if (virDomainDefMaybeAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_SCSI, i, -1) < 0) + if (virDomainDefMaybeAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_SCSI, + i, newModel) < 0) return -1; }
diff --git a/tests/qemuxml2xmloutdata/hostdev-scsi-autogen-address.xml b/tests/qemuxml2xmloutdata/hostdev-scsi-autogen-address.xml index 8e93056ee..cea212b64 100644 --- a/tests/qemuxml2xmloutdata/hostdev-scsi-autogen-address.xml +++ b/tests/qemuxml2xmloutdata/hostdev-scsi-autogen-address.xml @@ -29,7 +29,7 @@ <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> </controller> <controller type='pci' index='0' model='pci-root'/> - <controller type='scsi' index='1'> + <controller type='scsi' index='1' model='virtio-scsi'> <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> </controller> <input type='mouse' bus='ps2'/>
On 12/15/2017 11:32 AM, Eric Farman wrote:
On 12/06/2017 08:08 AM, John Ferlan wrote:
In virDomainDefMaybeAddHostdevSCSIcontroller when we add a new controller because someone neglected to add one or we're adding one because the existing one is full, we should copy over the model number from the existing controller since whatever we create should at least have the same characteristics as the one we cannot use because it's full.
NB: This affects the existing hostdev-scsi-autogen-address test which would add a default ('lsi') SCSI controller for the various scsi_host's that would create a controller for the hostdev.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 13 ++++++++++++- tests/qemuxml2xmloutdata/hostdev-scsi-autogen-address.xml | 2 +- 2 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 66e21c4bd..61b4a0075 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -17689,12 +17689,22 @@ virDomainDefMaybeAddHostdevSCSIcontroller(virDomainDefPtr def) size_t i; int maxController = -1; virDomainHostdevDefPtr hostdev; + virDomainControllerModelSCSI model = -1; + virDomainControllerModelSCSI newModel = -1;
for (i = 0; i < def->nhostdevs; i++) { hostdev = def->hostdevs[i]; if (virHostdevIsSCSIDevice(hostdev) && (int)hostdev->info->addr.drive.controller > maxController) { maxController = hostdev->info->addr.drive.controller; + /* We may be creating a new controller because this one is full. + * So let's grab the model from it and update the model we're + * going to add as long as this one isn't undefined. The premise + * being keeping the same controller model for all SCSI hostdevs. */ + model = virDomainDeviceFindControllerModel(def, hostdev->info, + VIR_DOMAIN_CONTROLLER_TYPE_SCSI); + if (model != -1) + newModel = model;
What's the harm if the last controller were undefined? Wouldn't it get populated down the road anyway if one were not set at this point (we send -1 today, after all). That could eliminate one of the two virDomainControllerModelSCSI variables here, I would think.
Yes, a -1 is passed along which (for now) means a SCSI LSI controller would be created except for pseries which would generate something different. John
But, either way I think it's fine.
Reviewed-by: Eric Farman <farman@linux.vnet.ibm.com>
} }
@@ -17702,7 +17712,8 @@ virDomainDefMaybeAddHostdevSCSIcontroller(virDomainDefPtr def) return 0;
for (i = 0; i <= maxController; i++) { - if (virDomainDefMaybeAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_SCSI, i, -1) < 0) + if (virDomainDefMaybeAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_SCSI, + i, newModel) < 0) return -1; }
diff --git a/tests/qemuxml2xmloutdata/hostdev-scsi-autogen-address.xml b/tests/qemuxml2xmloutdata/hostdev-scsi-autogen-address.xml index 8e93056ee..cea212b64 100644 --- a/tests/qemuxml2xmloutdata/hostdev-scsi-autogen-address.xml +++ b/tests/qemuxml2xmloutdata/hostdev-scsi-autogen-address.xml @@ -29,7 +29,7 @@ <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> </controller> <controller type='pci' index='0' model='pci-root'/> - <controller type='scsi' index='1'> + <controller type='scsi' index='1' model='virtio-scsi'> <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> </controller> <input type='mouse' bus='ps2'/>
On Wed, Dec 06, 2017 at 08:08:05AM -0500, John Ferlan wrote:
In virDomainDefMaybeAddHostdevSCSIcontroller when we add a new controller because someone neglected to add one or we're adding one because the existing one is full, we should copy over the model number from the existing controller since whatever we create should at least have the same characteristics as the one we cannot use because it's full.
NB: This affects the existing hostdev-scsi-autogen-address test which would add a default ('lsi') SCSI controller for the various scsi_host's that would create a controller for the hostdev.
Yet the change adds 'virtio-scsi'.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 13 ++++++++++++- tests/qemuxml2xmloutdata/hostdev-scsi-autogen-address.xml | 2 +- 2 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 66e21c4bd..61b4a0075 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -17689,12 +17689,22 @@ virDomainDefMaybeAddHostdevSCSIcontroller(virDomainDefPtr def) size_t i; int maxController = -1; virDomainHostdevDefPtr hostdev; + virDomainControllerModelSCSI model = -1;
This declaration can be moved inside the if to reduce its scope. (I'm with Andrea on this [0])
+ virDomainControllerModelSCSI newModel = -1;
for (i = 0; i < def->nhostdevs; i++) { hostdev = def->hostdevs[i]; if (virHostdevIsSCSIDevice(hostdev) && (int)hostdev->info->addr.drive.controller > maxController) { maxController = hostdev->info->addr.drive.controller; + /* We may be creating a new controller because this one is full. + * So let's grab the model from it and update the model we're + * going to add as long as this one isn't undefined. The premise + * being keeping the same controller model for all SCSI hostdevs. */ + model = virDomainDeviceFindControllerModel(def, hostdev->info, + VIR_DOMAIN_CONTROLLER_TYPE_SCSI); + if (model != -1) + newModel = model; } }
ACK Jan [0] https://www.redhat.com/archives/libvir-list/2017-December/msg00563.html
On Wed, Dec 20, 2017 at 01:52:16PM +0100, Ján Tomko wrote:
On Wed, Dec 06, 2017 at 08:08:05AM -0500, John Ferlan wrote:
In virDomainDefMaybeAddHostdevSCSIcontroller when we add a new controller because someone neglected to add one or we're adding one because the existing one is full, we should copy over the model number from the existing controller since whatever we create should at least have the same characteristics as the one we cannot use because it's full.
NB: This affects the existing hostdev-scsi-autogen-address test which would add a default ('lsi') SCSI controller for the various scsi_host's that would create a controller for the hostdev.
Yet the change adds 'virtio-scsi'.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 13 ++++++++++++- tests/qemuxml2xmloutdata/hostdev-scsi-autogen-address.xml | 2 +- 2 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 66e21c4bd..61b4a0075 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -17689,12 +17689,22 @@ virDomainDefMaybeAddHostdevSCSIcontroller(virDomainDefPtr def) size_t i; int maxController = -1; virDomainHostdevDefPtr hostdev; + virDomainControllerModelSCSI model = -1;
This declaration can be moved inside the if to reduce its scope. (I'm with Andrea on this [0])
Also, assigning -1 to an unsigned variable won't stick.
+ virDomainControllerModelSCSI newModel = -1;
for (i = 0; i < def->nhostdevs; i++) { hostdev = def->hostdevs[i]; if (virHostdevIsSCSIDevice(hostdev) && (int)hostdev->info->addr.drive.controller > maxController) { maxController = hostdev->info->addr.drive.controller; + /* We may be creating a new controller because this one is full. + * So let's grab the model from it and update the model we're + * going to add as long as this one isn't undefined. The premise + * being keeping the same controller model for all SCSI hostdevs. */ + model = virDomainDeviceFindControllerModel(def, hostdev->info, + VIR_DOMAIN_CONTROLLER_TYPE_SCSI); + if (model != -1)
This condition is always true according to my compiler. Jan
+ newModel = model; } }
ACK
Jan
[0] https://www.redhat.com/archives/libvir-list/2017-December/msg00563.html
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
https://bugzilla.redhat.com/show_bug.cgi?id=1519130 Commit id 'dc692438' reverted the automagic addition of a SCSI controller attempt during virDomainHostdevAssignAddress; however, the logic to determine where to place the next_unit depended upon the "new" controller being added. Without the new controller the the next time through the call for the next SCSI hostdev found would result in the "next_unit" never changing from 0 (zero) and as a result the addition of the device will fail due to being a duplicate unit number of the first with the error message: virDomainDefCheckDuplicateDriveAddresses:$line : unsupported configuration: SCSI host address controller='0' bus='1' target='0' unit='0' in use by another SCSI host device So instead of walking the controller list looking for SCSI controllers, all we can do is "pretend" that they exist and allow other code to create them later as necessary. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 31 +++++++++++++------------------ 1 file changed, 13 insertions(+), 18 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 61b4a0075..73c6708cf 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4322,10 +4322,8 @@ virDomainHostdevAssignAddress(virDomainXMLOptionPtr xmlopt, const virDomainDef *def, virDomainHostdevDefPtr hostdev) { - int next_unit = 0; - unsigned controller = 0; + int controller = 0; unsigned int max_unit; - size_t i; int ret; if (xmlopt->config.features & VIR_DOMAIN_DEF_FEATURE_WIDE_SCSI) @@ -4333,33 +4331,30 @@ virDomainHostdevAssignAddress(virDomainXMLOptionPtr xmlopt, else max_unit = SCSI_NARROW_BUS_MAX_CONT_UNIT; - for (i = 0; i < def->ncontrollers; i++) { - if (def->controllers[i]->type != VIR_DOMAIN_CONTROLLER_TYPE_SCSI) - continue; - - controller++; - ret = virDomainControllerSCSINextUnit(def, max_unit, - def->controllers[i]->idx); - if (ret >= 0) { - next_unit = ret; - controller = def->controllers[i]->idx; - break; - } - } - /* 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. + * + * Because we cannot add a controller, then we should not walk the + * defined controllers list in order to find empty space. Doing + * so fails to return the valid next unit number for the 2nd + * hostdev being added to the as yet to be created controller. */ + do { + ret = virDomainControllerSCSINextUnit(def, max_unit, controller); + if (ret < 0) + controller++; + } while (ret < 0); + hostdev->info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE; hostdev->info->addr.drive.controller = controller; hostdev->info->addr.drive.bus = 0; hostdev->info->addr.drive.target = 0; - hostdev->info->addr.drive.unit = next_unit; + hostdev->info->addr.drive.unit = ret; return 0; } -- 2.13.6
On 12/06/2017 08:08 AM, John Ferlan wrote:
Commit id 'dc692438' reverted the automagic addition of a SCSI controller attempt during virDomainHostdevAssignAddress; however, the logic to determine where to place the next_unit depended upon the "new" controller being added. Without the new controller the the next time through the call for the next SCSI hostdev found would result in the "next_unit" never changing from 0 (zero) and as a result the addition of the device will fail due to being a duplicate unit number of the first with the error message:
virDomainDefCheckDuplicateDriveAddresses:$line : unsupported configuration: SCSI host address controller='0' bus='1' target='0' unit='0' in use by another SCSI host device
So instead of walking the controller list looking for SCSI controllers, all we can do is "pretend" that they exist and allow other code to create them later as necessary.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 31 +++++++++++++------------------ 1 file changed, 13 insertions(+), 18 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 61b4a0075..73c6708cf 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4322,10 +4322,8 @@ virDomainHostdevAssignAddress(virDomainXMLOptionPtr xmlopt, const virDomainDef *def, virDomainHostdevDefPtr hostdev) { - int next_unit = 0; - unsigned controller = 0; + int controller = 0; unsigned int max_unit; - size_t i; int ret;
if (xmlopt->config.features & VIR_DOMAIN_DEF_FEATURE_WIDE_SCSI) @@ -4333,33 +4331,30 @@ virDomainHostdevAssignAddress(virDomainXMLOptionPtr xmlopt, else max_unit = SCSI_NARROW_BUS_MAX_CONT_UNIT;
- for (i = 0; i < def->ncontrollers; i++) { - if (def->controllers[i]->type != VIR_DOMAIN_CONTROLLER_TYPE_SCSI) - continue;
Don't we still need this check in the case of non-SCSI controllers intermixed with SCSI ones?
- - controller++; - ret = virDomainControllerSCSINextUnit(def, max_unit, - def->controllers[i]->idx); - if (ret >= 0) { - next_unit = ret; - controller = def->controllers[i]->idx; - break; - } - } - /* 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. + * + * Because we cannot add a controller, then we should not walk the + * defined controllers list in order to find empty space.
But we do walk the list, we just don't use a for loop to do it.
Doing + * so fails to return the valid next unit number for the 2nd + * hostdev being added to the as yet to be created controller. */ + do { + ret = virDomainControllerSCSINextUnit(def, max_unit, controller); + if (ret < 0) + controller++; + } while (ret < 0); +
I do like the simplification of the loop though! - Eric
hostdev->info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE; hostdev->info->addr.drive.controller = controller; hostdev->info->addr.drive.bus = 0; hostdev->info->addr.drive.target = 0; - hostdev->info->addr.drive.unit = next_unit; + hostdev->info->addr.drive.unit = ret;
return 0; }
On 12/15/2017 11:32 AM, Eric Farman wrote:
On 12/06/2017 08:08 AM, John Ferlan wrote:
Commit id 'dc692438' reverted the automagic addition of a SCSI controller attempt during virDomainHostdevAssignAddress; however, the logic to determine where to place the next_unit depended upon the "new" controller being added. Without the new controller the the next time through the call for the next SCSI hostdev found would result in the "next_unit" never changing from 0 (zero) and as a result the addition of the device will fail due to being a duplicate unit number of the first with the error message:
virDomainDefCheckDuplicateDriveAddresses:$line : unsupported configuration: SCSI host address controller='0' bus='1' target='0' unit='0' in use by another SCSI host device
So instead of walking the controller list looking for SCSI controllers, all we can do is "pretend" that they exist and allow other code to create them later as necessary.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 31 +++++++++++++------------------ 1 file changed, 13 insertions(+), 18 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 61b4a0075..73c6708cf 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4322,10 +4322,8 @@ virDomainHostdevAssignAddress(virDomainXMLOptionPtr xmlopt, const virDomainDef *def, virDomainHostdevDefPtr hostdev) { - int next_unit = 0; - unsigned controller = 0; + int controller = 0; unsigned int max_unit; - size_t i; int ret;
if (xmlopt->config.features & VIR_DOMAIN_DEF_FEATURE_WIDE_SCSI) @@ -4333,33 +4331,30 @@ virDomainHostdevAssignAddress(virDomainXMLOptionPtr xmlopt, else max_unit = SCSI_NARROW_BUS_MAX_CONT_UNIT;
- for (i = 0; i < def->ncontrollers; i++) { - if (def->controllers[i]->type != VIR_DOMAIN_CONTROLLER_TYPE_SCSI) - continue;
Don't we still need this check in the case of non-SCSI controllers intermixed with SCSI ones?
This API is called from virDomainHostdevDefPostParse only when the <hostdev> 'type' is VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI when the <address> 'type' is VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE. So we're already limited to a very specific set. This would have been needed if we were running through all the controllers because we couldn't add to a non SCSI controller.
- - controller++; - ret = virDomainControllerSCSINextUnit(def, max_unit, - def->controllers[i]->idx); - if (ret >= 0) { - next_unit = ret; - controller = def->controllers[i]->idx; - break; - } - } - /* 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. + * + * Because we cannot add a controller, then we should not walk the + * defined controllers list in order to find empty space.
But we do walk the list, we just don't use a for loop to do it.
We're not really walking the controller list, we're in a function that is being called for every hostdev in the 'nhostdevs' list. John
Doing + * so fails to return the valid next unit number for the 2nd + * hostdev being added to the as yet to be created controller. */ + do { + ret = virDomainControllerSCSINextUnit(def, max_unit, controller); + if (ret < 0) + controller++; + } while (ret < 0); +
I do like the simplification of the loop though!
- Eric
hostdev->info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE; hostdev->info->addr.drive.controller = controller; hostdev->info->addr.drive.bus = 0; hostdev->info->addr.drive.target = 0; - hostdev->info->addr.drive.unit = next_unit; + hostdev->info->addr.drive.unit = ret;
return 0; }
On Wed, Dec 06, 2017 at 08:08:06AM -0500, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1519130
Commit id 'dc692438' reverted the automagic addition of a SCSI controller attempt during virDomainHostdevAssignAddress; however, the logic to determine where to place the next_unit depended upon the "new" controller being added. Without the new controller the the next time through the call for the next SCSI hostdev found would result in the "next_unit" never changing from 0 (zero) and as a result the addition of the device will fail due to being a duplicate unit number of the first with the error message:
virDomainDefCheckDuplicateDriveAddresses:$line : unsupported configuration: SCSI host address controller='0' bus='1' target='0' unit='0' in use by another SCSI host device
So instead of walking the controller list looking for SCSI controllers, all we can do is "pretend" that they exist and allow other code to create them later as necessary.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 31 +++++++++++++------------------ 1 file changed, 13 insertions(+), 18 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 61b4a0075..73c6708cf 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4322,10 +4322,8 @@ virDomainHostdevAssignAddress(virDomainXMLOptionPtr xmlopt, const virDomainDef *def, virDomainHostdevDefPtr hostdev) { - int next_unit = 0;
Please keep the descriptive 'next_unit' variable name.
- unsigned controller = 0; + int controller = 0; unsigned int max_unit; - size_t i; int ret;
if (xmlopt->config.features & VIR_DOMAIN_DEF_FEATURE_WIDE_SCSI) @@ -4333,33 +4331,30 @@ virDomainHostdevAssignAddress(virDomainXMLOptionPtr xmlopt, else max_unit = SCSI_NARROW_BUS_MAX_CONT_UNIT;
- for (i = 0; i < def->ncontrollers; i++) { - if (def->controllers[i]->type != VIR_DOMAIN_CONTROLLER_TYPE_SCSI) - continue; - - controller++; - ret = virDomainControllerSCSINextUnit(def, max_unit, - def->controllers[i]->idx); - if (ret >= 0) { - next_unit = ret; - controller = def->controllers[i]->idx; - break; - } - } - /* 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. + * + * Because we cannot add a controller, then we should not walk the + * defined controllers list in order to find empty space. Doing + * so fails to return the valid next unit number for the 2nd + * hostdev being added to the as yet to be created controller. */ + do { + ret = virDomainControllerSCSINextUnit(def, max_unit, controller); + if (ret < 0) + controller++; + } while (ret < 0); +
Easier to read as: for (next_unit = -1; next_unit < -1; controller++) next_unit = virDomainControllerSCSINextUnit(def, max_unit, controller); ACK Jan
hostdev->info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE; hostdev->info->addr.drive.controller = controller; hostdev->info->addr.drive.bus = 0; hostdev->info->addr.drive.target = 0; - hostdev->info->addr.drive.unit = next_unit; + hostdev->info->addr.drive.unit = ret;
return 0; } -- 2.13.6
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 12/20/2017 07:38 AM, Ján Tomko wrote:
On Wed, Dec 06, 2017 at 08:08:06AM -0500, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1519130
Commit id 'dc692438' reverted the automagic addition of a SCSI controller attempt during virDomainHostdevAssignAddress; however, the logic to determine where to place the next_unit depended upon the "new" controller being added. Without the new controller the the next time through the call for the next SCSI hostdev found would result in the "next_unit" never changing from 0 (zero) and as a result the addition of the device will fail due to being a duplicate unit number of the first with the error message:
virDomainDefCheckDuplicateDriveAddresses:$line : unsupported configuration: SCSI host address controller='0' bus='1' target='0' unit='0' in use by another SCSI host device
So instead of walking the controller list looking for SCSI controllers, all we can do is "pretend" that they exist and allow other code to create them later as necessary.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 31 +++++++++++++------------------ 1 file changed, 13 insertions(+), 18 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 61b4a0075..73c6708cf 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4322,10 +4322,8 @@ virDomainHostdevAssignAddress(virDomainXMLOptionPtr xmlopt, const virDomainDef *def, virDomainHostdevDefPtr hostdev) { - int next_unit = 0;
Please keep the descriptive 'next_unit' variable name.
- unsigned controller = 0; + int controller = 0; unsigned int max_unit; - size_t i; int ret;
if (xmlopt->config.features & VIR_DOMAIN_DEF_FEATURE_WIDE_SCSI) @@ -4333,33 +4331,30 @@ virDomainHostdevAssignAddress(virDomainXMLOptionPtr xmlopt, else max_unit = SCSI_NARROW_BUS_MAX_CONT_UNIT;
- for (i = 0; i < def->ncontrollers; i++) { - if (def->controllers[i]->type != VIR_DOMAIN_CONTROLLER_TYPE_SCSI) - continue; - - controller++; - ret = virDomainControllerSCSINextUnit(def, max_unit, - def->controllers[i]->idx); - if (ret >= 0) { - next_unit = ret; - controller = def->controllers[i]->idx; - break; - } - } - /* 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. + * + * Because we cannot add a controller, then we should not walk the + * defined controllers list in order to find empty space. Doing + * so fails to return the valid next unit number for the 2nd + * hostdev being added to the as yet to be created controller. */ + do { + ret = virDomainControllerSCSINextUnit(def, max_unit, controller); + if (ret < 0) + controller++; + } while (ret < 0); +
Easier to read as: for (next_unit = -1; next_unit < -1; controller++) next_unit = virDomainControllerSCSINextUnit(def, max_unit, controller);
Not functionally the same comparisons... Caused hostdev-scsi-autogen-address to fail... There's 11 hostdevs and only 1 controller. We never get controller==1 from that for loop. I can change @ret above to be @next_unit though John
ACK
Jan
hostdev->info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE; hostdev->info->addr.drive.controller = controller; hostdev->info->addr.drive.bus = 0; hostdev->info->addr.drive.target = 0; - hostdev->info->addr.drive.unit = next_unit; + hostdev->info->addr.drive.unit = ret;
return 0; } -- 2.13.6
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Jan - ping on the question from my response to your review... On 12/20/2017 01:33 PM, John Ferlan wrote:
On 12/20/2017 07:38 AM, Ján Tomko wrote:
On Wed, Dec 06, 2017 at 08:08:06AM -0500, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1519130
Commit id 'dc692438' reverted the automagic addition of a SCSI controller attempt during virDomainHostdevAssignAddress; however, the logic to determine where to place the next_unit depended upon the "new" controller being added. Without the new controller the the next time through the call for the next SCSI hostdev found would result in the "next_unit" never changing from 0 (zero) and as a result the addition of the device will fail due to being a duplicate unit number of the first with the error message:
virDomainDefCheckDuplicateDriveAddresses:$line : unsupported configuration: SCSI host address controller='0' bus='1' target='0' unit='0' in use by another SCSI host device
So instead of walking the controller list looking for SCSI controllers, all we can do is "pretend" that they exist and allow other code to create them later as necessary.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 31 +++++++++++++------------------ 1 file changed, 13 insertions(+), 18 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 61b4a0075..73c6708cf 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4322,10 +4322,8 @@ virDomainHostdevAssignAddress(virDomainXMLOptionPtr xmlopt, const virDomainDef *def, virDomainHostdevDefPtr hostdev) { - int next_unit = 0;
Please keep the descriptive 'next_unit' variable name.
- unsigned controller = 0; + int controller = 0; unsigned int max_unit; - size_t i; int ret;
if (xmlopt->config.features & VIR_DOMAIN_DEF_FEATURE_WIDE_SCSI) @@ -4333,33 +4331,30 @@ virDomainHostdevAssignAddress(virDomainXMLOptionPtr xmlopt, else max_unit = SCSI_NARROW_BUS_MAX_CONT_UNIT;
- for (i = 0; i < def->ncontrollers; i++) { - if (def->controllers[i]->type != VIR_DOMAIN_CONTROLLER_TYPE_SCSI) - continue; - - controller++; - ret = virDomainControllerSCSINextUnit(def, max_unit, - def->controllers[i]->idx); - if (ret >= 0) { - next_unit = ret; - controller = def->controllers[i]->idx; - break; - } - } - /* 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. + * + * Because we cannot add a controller, then we should not walk the + * defined controllers list in order to find empty space. Doing + * so fails to return the valid next unit number for the 2nd + * hostdev being added to the as yet to be created controller. */ + do { + ret = virDomainControllerSCSINextUnit(def, max_unit, controller); + if (ret < 0) + controller++; + } while (ret < 0); +
Easier to read as: for (next_unit = -1; next_unit < -1; controller++) next_unit = virDomainControllerSCSINextUnit(def, max_unit, controller);
Not functionally the same comparisons... Caused hostdev-scsi-autogen-address to fail... There's 11 hostdevs and only 1 controller. We never get controller==1 from that for loop.
I can change @ret above to be @next_unit though
Is changing to use next_unit enough? Or did you have some other easier to read loop that actually works that you'd prefer to see? Tks -
John
ACK
Jan
hostdev->info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE; hostdev->info->addr.drive.controller = controller; hostdev->info->addr.drive.bus = 0; hostdev->info->addr.drive.target = 0; - hostdev->info->addr.drive.unit = next_unit; + hostdev->info->addr.drive.unit = ret;
return 0; } -- 2.13.6
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Jan 04, 2018 at 07:24:30AM -0500, John Ferlan wrote:
Jan -
ping on the question from my response to your review...
On 12/20/2017 01:33 PM, John Ferlan wrote:
On 12/20/2017 07:38 AM, Ján Tomko wrote:
[...]
Easier to read as: for (next_unit = -1; next_unit < -1; controller++) next_unit = virDomainControllerSCSINextUnit(def, max_unit, controller);
Not functionally the same comparisons... Caused hostdev-scsi-autogen-address to fail... There's 11 hostdevs and only 1 controller. We never get controller==1 from that for loop.
I can change @ret above to be @next_unit though
Is changing to use next_unit enough?
Yes.
Or did you have some other easier to read loop that actually works that you'd prefer to see?
I do not. Jan
ping? Tks - John On 12/06/2017 08:08 AM, John Ferlan wrote:
Details in the various patches.
John Ferlan (4): qemu: Tolerate storage source private data being NULL for hotplug SCSI hostdev qemu: Use same model when adding hostdev SCSI controller conf: Use existing SCSI hostdev model to create new conf: Fix generating addresses for SCSI hostdev
src/conf/domain_conf.c | 44 ++++++++++++---------- src/qemu/qemu_hotplug.c | 19 +++++++--- .../hostdev-scsi-autogen-address.xml | 2 +- 3 files changed, 39 insertions(+), 26 deletions(-)
participants (3)
-
Eric Farman -
John Ferlan -
Ján Tomko