Move the checks that various attributes are not set on any controller
other than SCSI controller using virtio-scsi model into the common
controller validate checks.
Signed-off-by: John Ferlan <jferlan@redhat.com>
---
src/qemu/qemu_command.c | 24 ------------------------
src/qemu/qemu_domain.c | 41 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 41 insertions(+), 24 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 2dd50a214..d9cbdff83 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -2667,30 +2667,6 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef,
return -1;
}
- if (!(def->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI &&
- model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI)) {
- if (def->queues) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("'queues' is only supported by virtio-scsi controller"));
- return -1;
- }
- if (def->cmd_per_lun) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("'cmd_per_lun' is only supported by virtio-scsi controller"));
- return -1;
- }
- if (def->max_sectors) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("'max_sectors' is only supported by virtio-scsi controller"));
- return -1;
- }
- if (def->ioeventfd) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("'ioeventfd' is only supported by virtio-scsi controller"));
- return -1;
- }
- }
-
switch ((virDomainControllerType) def->type) {
case VIR_DOMAIN_CONTROLLER_TYPE_SCSI:
switch (model) {
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 347fc0742..120c013bd 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -3893,6 +3893,38 @@ qemuDomainDeviceDefValidateDisk(const virDomainDiskDef *disk)
static int
+qemuDomainDeviceDefValidateControllerAttributes(const virDomainControllerDef *controller,
+ int model)
+{
+ if (!(controller->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI &&
+ model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI)) {
+ if (controller->queues) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("'queues' is only supported by virtio-scsi controller"));
+ return -1;
+ }
+ if (controller->cmd_per_lun) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("'cmd_per_lun' is only supported by virtio-scsi controller"));
+ return -1;
+ }
+ if (controller->max_sectors) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("'max_sectors' is only supported by virtio-scsi controller"));
+ return -1;
+ }
+ if (controller->ioeventfd) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("'ioeventfd' is only supported by virtio-scsi controller"));
+ return -1;
+ }
+ }
+
+ return 0;
+}
+
+
+static int
qemuDomainDeviceDefValidateControllerIDE(const virDomainControllerDef *controller,
const virDomainDef *def)
{
@@ -3924,11 +3956,20 @@ qemuDomainDeviceDefValidateController(const virDomainControllerDef *controller,
virQEMUCapsPtr qemuCaps)
{
int ret = 0;
+ int model = controller->model;
if (!qemuDomainCheckCCWS390AddressSupport(def, controller->info, qemuCaps,
"controller"))
return -1;
+ if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) {
+ if ((qemuDomainSetSCSIControllerModel(def, qemuCaps, &model)) < 0)
+ return -1;
+ }
+
+ if (qemuDomainDeviceDefValidateControllerAttributes(controller, model) < 0)
+ return -1;
+
switch ((virDomainControllerType) controller->type) {
case VIR_DOMAIN_CONTROLLER_TYPE_IDE:
ret = qemuDomainDeviceDefValidateControllerIDE(controller, def);
--
2.13.6
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Dec 12, 2017 at 04:06 PM +0100, John Ferlan <jferlan@redhat.com> wrote:
> Move the checks that various attributes are not set on any controller
> other than SCSI controller using virtio-scsi model into the common
> controller validate checks.
You not only move the checks, but also add the call
'qemuDomainSetSCSIControllerModel'.
>
> Signed-off-by: John Ferlan <jferlan@redhat.com>
> ---
> src/qemu/qemu_command.c | 24 ------------------------
> src/qemu/qemu_domain.c | 41 +++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 41 insertions(+), 24 deletions(-)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 2dd50a214..d9cbdff83 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -2667,30 +2667,6 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef,
> return -1;
> }
>
> - if (!(def->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI &&
> - model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI)) {
> - if (def->queues) {
> - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> - _("'queues' is only supported by virtio-scsi controller"));
> - return -1;
> - }
> - if (def->cmd_per_lun) {
> - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> - _("'cmd_per_lun' is only supported by virtio-scsi controller"));
> - return -1;
> - }
> - if (def->max_sectors) {
> - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> - _("'max_sectors' is only supported by virtio-scsi controller"));
> - return -1;
> - }
> - if (def->ioeventfd) {
> - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> - _("'ioeventfd' is only supported by virtio-scsi controller"));
> - return -1;
> - }
> - }
> -
> switch ((virDomainControllerType) def->type) {
> case VIR_DOMAIN_CONTROLLER_TYPE_SCSI:
> switch (model) {
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 347fc0742..120c013bd 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -3893,6 +3893,38 @@ qemuDomainDeviceDefValidateDisk(const virDomainDiskDef *disk)
>
>
> static int
> +qemuDomainDeviceDefValidateControllerAttributes(const virDomainControllerDef *controller,
> + int model)
> +{
> + if (!(controller->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI &&
> + model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI)) {
> + if (controller->queues) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("'queues' is only supported by virtio-scsi controller"));
> + return -1;
> + }
> + if (controller->cmd_per_lun) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("'cmd_per_lun' is only supported by virtio-scsi controller"));
> + return -1;
> + }
> + if (controller->max_sectors) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("'max_sectors' is only supported by virtio-scsi controller"));
> + return -1;
> + }
> + if (controller->ioeventfd) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("'ioeventfd' is only supported by virtio-scsi controller"));
> + return -1;
> + }
> + }
> +
> + return 0;
> +}
> +
> +
> +static int
> qemuDomainDeviceDefValidateControllerIDE(const virDomainControllerDef *controller,
> const virDomainDef *def)
> {
> @@ -3924,11 +3956,20 @@ qemuDomainDeviceDefValidateController(const virDomainControllerDef *controller,
> virQEMUCapsPtr qemuCaps)
> {
> int ret = 0;
> + int model = controller->model;
>
> if (!qemuDomainCheckCCWS390AddressSupport(def, controller->info, qemuCaps,
> "controller"))
> return -1;
>
> + if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) {
> + if ((qemuDomainSetSCSIControllerModel(def, qemuCaps, &model)) < 0)
> + return -1;
> + }
Didn't take a closer look, but is it the right place for this? (in a
validation function)
> +
> + if (qemuDomainDeviceDefValidateControllerAttributes(controller, model) < 0)
> + return -1;
> +
> switch ((virDomainControllerType) controller->type) {
> case VIR_DOMAIN_CONTROLLER_TYPE_IDE:
> ret = qemuDomainDeviceDefValidateControllerIDE(controller, def);
> --
> 2.13.6
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
--
Beste Grüße / Kind regards
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 01/04/2018 05:23 AM, Marc Hartmayer wrote:
> On Tue, Dec 12, 2017 at 04:06 PM +0100, John Ferlan <jferlan@redhat.com> wrote:
>> Move the checks that various attributes are not set on any controller
>> other than SCSI controller using virtio-scsi model into the common
>> controller validate checks.
>
> You not only move the checks, but also add the call
> 'qemuDomainSetSCSIControllerModel'.
>
True, but it's required... Something noted in the v3 review regarding
that this Set function is more like a Get function and a question over
whether we could move it to post-parse. The question and my thoughts
from that cut-n-pasted here since this is the more recent review:
>
> After this patch, this function is called twice. Also it's called
> SetModel, while it's behaving as GetModel. Can we start assigning the
> default model in post-parse without breaking backwards compatibility?
>
> Jan
>
Are you asking that during PostParse callback for us to set def->model
so that we can just use it later on? That'd make things a lot easier
later on, but that seems a bit outside of what I was trying to do though.
Anyway based on this series and another I have posted... That means for
someone that didn't set the model, that a model would then be set and
written out... Meaning adjustment of a number of tests because now there
would be a model defined. It also could mean we cannot adjust the
default model - one could say for example that using lsi as the default
model for scsi_host passthrough is perhaps less optimal than using
virtio-scsi... In fact, based on a different series and some testing
done - it may not work either.
As for GetModel vs. SetModel - I think that's a different problem and
can be put on a list of things to do after this is done.
...
>>
>> Signed-off-by: John Ferlan <jferlan@redhat.com>
>> ---
>> src/qemu/qemu_command.c | 24 ------------------------
>> src/qemu/qemu_domain.c | 41 +++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 41 insertions(+), 24 deletions(-)
>>
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index 2dd50a214..d9cbdff83 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -2667,30 +2667,6 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef,
>> return -1;
>> }
>>
>> - if (!(def->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI &&
>> - model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI)) {
>> - if (def->queues) {
>> - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> - _("'queues' is only supported by virtio-scsi controller"));
>> - return -1;
>> - }
>> - if (def->cmd_per_lun) {
>> - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> - _("'cmd_per_lun' is only supported by virtio-scsi controller"));
>> - return -1;
>> - }
>> - if (def->max_sectors) {
>> - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> - _("'max_sectors' is only supported by virtio-scsi controller"));
>> - return -1;
>> - }
>> - if (def->ioeventfd) {
>> - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> - _("'ioeventfd' is only supported by virtio-scsi controller"));
>> - return -1;
>> - }
>> - }
>> -
>> switch ((virDomainControllerType) def->type) {
>> case VIR_DOMAIN_CONTROLLER_TYPE_SCSI:
>> switch (model) {
>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> index 347fc0742..120c013bd 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -3893,6 +3893,38 @@ qemuDomainDeviceDefValidateDisk(const virDomainDiskDef *disk)
>>
>>
>> static int
>> +qemuDomainDeviceDefValidateControllerAttributes(const virDomainControllerDef *controller,
>> + int model)
>> +{
>> + if (!(controller->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI &&
>> + model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI)) {
>> + if (controller->queues) {
>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> + _("'queues' is only supported by virtio-scsi controller"));
>> + return -1;
>> + }
>> + if (controller->cmd_per_lun) {
>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> + _("'cmd_per_lun' is only supported by virtio-scsi controller"));
>> + return -1;
>> + }
>> + if (controller->max_sectors) {
>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> + _("'max_sectors' is only supported by virtio-scsi controller"));
>> + return -1;
>> + }
>> + if (controller->ioeventfd) {
>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> + _("'ioeventfd' is only supported by virtio-scsi controller"));
>> + return -1;
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +
>> +static int
>> qemuDomainDeviceDefValidateControllerIDE(const virDomainControllerDef *controller,
>> const virDomainDef *def)
>> {
>> @@ -3924,11 +3956,20 @@ qemuDomainDeviceDefValidateController(const virDomainControllerDef *controller,
>> virQEMUCapsPtr qemuCaps)
>> {
>> int ret = 0;
>> + int model = controller->model;
>>
>> if (!qemuDomainCheckCCWS390AddressSupport(def, controller->info, qemuCaps,
>> "controller"))
>> return -1;
>>
>> + if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) {
>> + if ((qemuDomainSetSCSIControllerModel(def, qemuCaps, &model)) < 0)
>> + return -1;
>> + }
>
> Didn't take a closer look, but is it the right place for this? (in a
> validation function)
>
As noted above - this ends up being required in order to "get" the right
model type for SCSI because we don't set a default otherwise during post
parse. I'm somewhat conflicted over what to do and how much (more)
effort to make to this series which I originally thought would be a
simple move from one place to another, but now is taking on a different
direction/life <sigh>...
John
>> +
>> + if (qemuDomainDeviceDefValidateControllerAttributes(controller, model) < 0)
>> + return -1;
>> +
>> switch ((virDomainControllerType) controller->type) {
>> case VIR_DOMAIN_CONTROLLER_TYPE_IDE:
>> ret = qemuDomainDeviceDefValidateControllerIDE(controller, def);
>> --
>> 2.13.6
>>
>> --
>> libvir-list mailing list
>> libvir-list@redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list
>>
> --
> Beste Grüße / Kind regards
> Marc Hartmayer
>
> IBM Deutschland Research & Development GmbH
> Vorsitzende des Aufsichtsrats: Martina Koederitz
> Geschäftsführung: Dirk Wittkopp
> Sitz der Gesellschaft: Böblingen
> Registergericht: Amtsgericht Stuttgart, HRB 243294
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Jan 04, 2018 at 02:12 PM +0100, John Ferlan <jferlan@redhat.com> wrote:
> On 01/04/2018 05:23 AM, Marc Hartmayer wrote:
>> On Tue, Dec 12, 2017 at 04:06 PM +0100, John Ferlan <jferlan@redhat.com> wrote:
>>> Move the checks that various attributes are not set on any controller
>>> other than SCSI controller using virtio-scsi model into the common
>>> controller validate checks.
>>
[…snip…]
>>> virQEMUCapsPtr qemuCaps)
>>> {
>>> int ret = 0;
>>> + int model = controller->model;
>>>
>>> if (!qemuDomainCheckCCWS390AddressSupport(def, controller->info, qemuCaps,
>>> "controller"))
>>> return -1;
>>>
>>> + if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) {
>>> + if ((qemuDomainSetSCSIControllerModel(def, qemuCaps, &model)) < 0)
>>> + return -1;
>>> + }
>>
>> Didn't take a closer look, but is it the right place for this? (in a
>> validation function)
>>
>
> As noted above - this ends up being required in order to "get" the right
> model type for SCSI because we don't set a default otherwise during post
> parse. I'm somewhat conflicted over what to do and how much (more)
> effort to make to this series which I originally thought would be a
> simple move from one place to another, but now is taking on a different
> direction/life <sigh>...
Hmm - if you want to you can leave your patch at it is. It’s already
much better than before.
But I would definitely split 'qemuDomainSetSCSIControllerModel' into two
separate functions as it does two distinct things. It either checks if
the passed controller model is supported by the QEMU caps or it sets the
controller model.
“As for GetModel vs. SetModel - I think that's a different problem and
can be put on a list of things to do after this is done.”
Do you mean with your comment the same as I suggested above?
>
> John
>
>>> +
>>> + if (qemuDomainDeviceDefValidateControllerAttributes(controller, model) < 0)
>>> + return -1;
>>> +
>>> switch ((virDomainControllerType) controller->type) {
>>> case VIR_DOMAIN_CONTROLLER_TYPE_IDE:
>>> ret = qemuDomainDeviceDefValidateControllerIDE(controller, def);
>>> --
>>> 2.13.6
>>>
>>> --
>>> libvir-list mailing list
>>> libvir-list@redhat.com
>>> https://www.redhat.com/mailman/listinfo/libvir-list
>>>
>> --
>> Beste Grüße / Kind regards
>> Marc Hartmayer
>>
>> IBM Deutschland Research & Development GmbH
>> Vorsitzende des Aufsichtsrats: Martina Koederitz
>> Geschäftsführung: Dirk Wittkopp
>> Sitz der Gesellschaft: Böblingen
>> Registergericht: Amtsgericht Stuttgart, HRB 243294
>>
>
--
Beste Grüße / Kind regards
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 01/05/2018 10:20 AM, Marc Hartmayer wrote:
> On Thu, Jan 04, 2018 at 02:12 PM +0100, John Ferlan <jferlan@redhat.com> wrote:
>> On 01/04/2018 05:23 AM, Marc Hartmayer wrote:
>>> On Tue, Dec 12, 2017 at 04:06 PM +0100, John Ferlan <jferlan@redhat.com> wrote:
>>>> Move the checks that various attributes are not set on any controller
>>>> other than SCSI controller using virtio-scsi model into the common
>>>> controller validate checks.
>>>
>
> […snip…]
>
>>>> virQEMUCapsPtr qemuCaps)
>>>> {
>>>> int ret = 0;
>>>> + int model = controller->model;
>>>>
>>>> if (!qemuDomainCheckCCWS390AddressSupport(def, controller->info, qemuCaps,
>>>> "controller"))
>>>> return -1;
>>>>
>>>> + if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) {
>>>> + if ((qemuDomainSetSCSIControllerModel(def, qemuCaps, &model)) < 0)
>>>> + return -1;
>>>> + }
>>>
>>> Didn't take a closer look, but is it the right place for this? (in a
>>> validation function)
>>>
>>
>> As noted above - this ends up being required in order to "get" the right
>> model type for SCSI because we don't set a default otherwise during post
>> parse. I'm somewhat conflicted over what to do and how much (more)
>> effort to make to this series which I originally thought would be a
>> simple move from one place to another, but now is taking on a different
>> direction/life <sigh>...
>
> Hmm - if you want to you can leave your patch at it is. It’s already
> much better than before.
>
> But I would definitely split 'qemuDomainSetSCSIControllerModel' into two
> separate functions as it does two distinct things. It either checks if
> the passed controller model is supported by the QEMU caps or it sets the
> controller model.
>
> “As for GetModel vs. SetModel - I think that's a different problem and
> can be put on a list of things to do after this is done.”
>
> Do you mean with your comment the same as I suggested above?
>
The split-up is done - there's a new series to include it as well as the
patch you posted...
Should make for hours of entertaining reading ;-)
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.