Re: [PATCH 03/10] conf: Set default scsi controller model when parsing

[Osier Yang <jyang@xxxxxxxxxx>]

On 2012年02月18日 00:41, Paolo Bonzini wrote:
> On 02/17/2012 06:18 PM, Osier Yang wrote:
> > And adding controller implicitly, ESX detects the default model
> > automatically itself, and partly depends on the guest os. So we
> > only try to set the default model for QEMU/KVM driver.
> > ---
> >   src/conf/domain_conf.c |   32 ++++++++++++++++++++++++++++++--
> >   1 files changed, 30 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > index 2da41f8..ccdb80d 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c
> > @@ -2502,6 +2502,28 @@ virDomainParseLegacyDeviceAddress(char *devaddr,
> >       return 0;
> >   }
> >
> > +static int
> > +virDomainControllerDefaultModel(virDomainDefPtr def,
> > +                                int type)
> > +{
> > +    switch(def->virtType) {
> > +    case VIR_DOMAIN_VIRT_TEST:
> > +    case VIR_DOMAIN_VIRT_QEMU:
> > +    case VIR_DOMAIN_VIRT_KQEMU:
> > +    case VIR_DOMAIN_VIRT_KVM:
> > +        if (type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) {
> > +            if (STREQ(def->os.arch, "ppc64")&&
> > +                STREQ(def->os.machine, "pseries")) {
> > +                return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_IBMVSCSI;
> > +            } else {
> > +                return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC;
> > +            }
> > +        }
> > +    default:
> > +       return -1;
> > +    }
> > +}
> > +
> >   int
> >   virDomainDiskDefAssignAddress(virCapsPtr caps, virDomainDiskDefPtr def)
> >   {
> > @@ -3469,6 +3491,7 @@ virDomainControllerModelTypeFromString(const virDomainControllerDefPtr def,
> >    */
> >   static virDomainControllerDefPtr
> >   virDomainControllerDefParseXML(xmlNodePtr node,
> > +                               virDomainDefPtr dom,
> >                                  unsigned int flags)
> >   {
> >       virDomainControllerDefPtr def;
> > @@ -3510,6 +3533,10 @@ virDomainControllerDefParseXML(xmlNodePtr node,
> >           def->model = -1;
> >       }
> >
> > +    if (def->model == -1 ||
> > +        def->model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_AUTO)
> > +        def->model = virDomainControllerDefaultModel(dom, def->type);
> > +
>
> I don't think this is correct.  In fact, I think I misunderstood you
> while we discussed this before by email.  So this patch is not necessary.

If you mean it's correct to check model in [PATCH 5/10] when formating
the XML. This patch is neccessary, otherwise we have no way to known
what model the driver sets by default, and thus no way to known how to
format the XML.

Even if we don't need to check the model when formating XML, I still
think this patch is useful. Why we set default model silently, and
don't show it up in XML? :-)

> Can you please resend the series without this patch, and with the test
> changes merged in the patch that affects the test output (presumably 4/10)?
>
> Paolo
>
> >       if (virDomainDeviceInfoParseXML(node, NULL,&def->info, flags)<  0)
> >           goto error;
> >
> > @@ -6562,7 +6589,7 @@ virDomainDeviceDefPtr virDomainDeviceDefParse(virCapsPtr caps,
> >               goto error;
> >       } else if (xmlStrEqual(node->name, BAD_CAST "controller")) {
> >           dev->type = VIR_DOMAIN_DEVICE_CONTROLLER;
> > -        if (!(dev->data.controller = virDomainControllerDefParseXML(node, flags)))
> > +        if (!(dev->data.controller = virDomainControllerDefParseXML(node, def, flags)))
> >               goto error;
> >       } else if (xmlStrEqual(node->name, BAD_CAST "graphics")) {
> >           dev->type = VIR_DOMAIN_DEVICE_GRAPHICS;
> > @@ -7124,7 +7151,7 @@ static int virDomainDefMaybeAddController(virDomainDefPtr def,
> >
> >       cont->type = type;
> >       cont->idx = idx;
> > -    cont->model = -1;
> > +    cont->model = virDomainControllerDefaultModel(def, type);
> >
> >       if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL) {
> >           cont->opts.vioserial.ports = -1;
> > @@ -7693,6 +7720,7 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps,
> >           goto no_memory;
> >       for (i = 0 ; i<  n ; i++) {
> >           virDomainControllerDefPtr controller = virDomainControllerDefParseXML(nodes[i],
> > +                                                                              def,
> >                                                                                 flags);
> >           if (!controller)
> >               goto error;

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list