[libvirt] [PATCH 06/34] Add new domain device: "controller"

[Daniel Veillard]

On Fri, Jan 15, 2010 at 02:08:54PM +0100, Daniel Veillard wrote:
> On Fri, Jan 08, 2010 at 05:23:02PM +0000, Daniel P. Berrange wrote:
> > From: Wolfgang Mauerer <wolfgang.mauerer at siemens.com>
> > 
> > This augments virDomainDevice with a <controller> element
> > that is used to represent disk controllers (e.g., scsi
> > controllers). The XML format is given by
> > 
> >   <controller type="scsi" index="<num>">
> >      <address type="pci" domain="0xNUM" bus="0xNUM" slot="0xNUM"/>
> >   </controller>
> 
>   Actually we also allow decimal or even octal values
> 
> > where type denotes the disk interface (scsi, ide,...), index
> > is an integer that identifies the controller for association
> > with disks, and the <address> element specifies the controller
> > address on the PCI bus as described in previous commits
> > The address element can be omitted; in this case, an address
> > will be assigned automatically.
> > 
> > Most of the code in this patch is from Wolfgang Mauerer's
> > previous disk controller series
> > 
> >  * docs/schemas/domain.rng: Define syntax for <controller>
> >    XML element
> >  * src/conf/domain_conf.c, src/conf/domain_conf.h: Define
> >    virDomainControllerDef struct, and routines for parsing
> >    and formatting XML
> > * src/libvirt_private.syms: Add virDomainControllerInsert
> >    and virDomainControllerDefFree
> [...]
> > +void virDomainControllerDefFree(virDomainControllerDefPtr def)
> > +{
> > +    if (!def)
> > +        return;
> > +
> > +    virDomainDeviceInfoClear(&def->info);
> 
>   superfluous, unless we start allocating data there.
> 
> > +    VIR_FREE(def);
> > +}
> > +
> [...]
> 
> > +/* Parse the XML definition for a controller
> > + * @param node XML nodeset to parse for controller definition
> > + */
> > +static virDomainControllerDefPtr
> > +virDomainControllerDefParseXML(virConnectPtr conn,
> > +                               xmlNodePtr node,
> > +                               int flags)
> > +{
> > +    virDomainControllerDefPtr def;
> > +    char *type = NULL;
> > +    char *idx = NULL;
> > +
> > +    if (VIR_ALLOC(def) < 0) {
> > +        virReportOOMError(conn);
> > +        return NULL;
> > +    }
> > +
> > +    type = virXMLPropString(node, "type");
> > +    if (type) {
> > +        if ((def->type = virDomainDiskBusTypeFromString(type)) < 0) {
> > +            virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR,
> > +                                 _("unknown disk controller type '%s'"), type);
> > +            goto error;
> > +        }
> > +    }
> > +
> > +    idx = virXMLPropString(node, "index");
> 
>   I like the code style found in the previous parsing routines where
> all the virXMLPropString are done at the beginning and we can drop
> the NULL initializations, looks simpler to me, but it's really not
> a big deal.
> 
> 
> > +void virDomainControllerInsertPreAlloced(virDomainDefPtr def,
> > +                                         virDomainControllerDefPtr controller)
> > +{
> > +    int i;
> > +    /* Tenatively plan to insert controller at the end. */
> > +    int insertAt = -1;
> > +
> > +    /* Then work backwards looking for controllers of
> > +     * the same type. If we find a controller with a
> > +     * index greater than the new one, insert at
> > +     * that position
> > +     */
> > +    for (i = (def->ncontrollers - 1) ; i >= 0 ; i--) {
> > +        /* If bus matches and current controller is after
> > +         * new controller, then new controller should go here */
> > +        if ((def->controllers[i]->type == controller->type) &&
> > +            (def->controllers[i]->idx > controller->idx)) {
> > +            insertAt = i;
> > +        } else if (def->controllers[i]->type == controller->type &&
> > +                   insertAt == -1) {
> > +            /* Last controller with match bus is before the
> > +             * new controller, then put new controller just after
> > +             */
> > +            insertAt = i + 1;
> > +        }
> > +    }
> > +
> > +    /* No controllers with this bus yet, so put at end of list */
> > +    if (insertAt == -1)
> > +        insertAt = def->ncontrollers;
> > +
> > +    if (insertAt < def->ncontrollers)
> > +        memmove(def->controllers + insertAt + 1,
> > +                def->controllers + insertAt,
> > +                (sizeof(def->controllers[0]) * (def->ncontrollers-insertAt)));
> > +
> > +    def->controllers[insertAt] = controller;
> > +    def->ncontrollers++;
> > +}
> 
>   I find that a bit complex, instead of trying to insert in an ordered
> fashion, what about another routine sorting the full set in one pass
> after the array had been fully initialized. We could use qsort and a
> single comparison function.
>   I think that would be equivalent, except controllers would be
> ordered by type too and we would loose that order if provided by the
> user. Unsure if this is a significant information.

  Hum, I realize I forgot to ack it, that's more a question than
  anything else

   ACK,

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/