Re: [PATCH v4 01/13] qemu: Introduce qemuDomainDeviceDefValidateControllerAttributes

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux