On 01/04/2018 05:23 AM, Marc Hartmayer wrote:
> On Tue, Dec 12, 2017 at 04:06 PM +0100, John Ferlan <jferlan@xxxxxxxxxx> 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@xxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxx
>> 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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
