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@xxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list