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

[Daniel Veillard]

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.

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/