On Thu, Apr 20, 2017 at 03:05:55PM +0200, Erik Skultety wrote: > Since there's at least SRIOV and MDEV sub-capabilities to be parsed, > let's make the code more readable by splitting it to several logical > blocks. > > Signed-off-by: Erik Skultety <eskul...@redhat.com> > --- > src/conf/node_device_conf.c | 84 > ++++++++++++++++++++++++++++----------------- > 1 file changed, 53 insertions(+), 31 deletions(-) > > diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c > index 85cfd8396..de346597a 100644 > --- a/src/conf/node_device_conf.c > +++ b/src/conf/node_device_conf.c > @@ -1286,76 +1286,100 @@ virPCIEDeviceInfoParseXML(xmlXPathContextPtr ctxt, > > > static int > -virNodeDevPCICapabilityParseXML(xmlXPathContextPtr ctxt, > - xmlNodePtr node, > - virNodeDevCapPCIDevPtr pci_dev) > +virNodeDevPCICapSRIOVParseXML(xmlXPathContextPtr ctxt, > + xmlNodePtr node, > + virNodeDevCapPCIDevPtr pci_dev, > + unsigned int cap) > {
You can go even further and split this function into two separate functions for "physical function" and "virtual function". > + int ret = -1; > char *maxFuncsStr = virXMLPropString(node, "maxCount"); > - char *type = virXMLPropString(node, "type"); > xmlNodePtr *addresses = NULL; > - xmlNodePtr orignode = ctxt->node; > - int ret = -1; > - size_t i = 0; > > - ctxt->node = node; > - > - if (!type) { > - virReportError(VIR_ERR_XML_ERROR, "%s", _("Missing capability > type")); > - goto out; > - } > - > - if (STREQ(type, "phys_function")) { > + if (cap == VIR_NODE_DEV_CAP_FLAG_PCI_PHYSICAL_FUNCTION) { > xmlNodePtr address = virXPathNode("./address[1]", ctxt); > > if (VIR_ALLOC(pci_dev->physical_function) < 0) > - goto out; > + goto cleanup; > > if (!address) { > virReportError(VIR_ERR_XML_ERROR, "%s", > _("Missing address in 'phys_function' > capability")); > - goto out; > + goto cleanup; > } > > if (virPCIDeviceAddressParseXML(address, > pci_dev->physical_function) < 0) > - goto out; > - > - pci_dev->flags |= VIR_NODE_DEV_CAP_FLAG_PCI_PHYSICAL_FUNCTION; > - } else if (STREQ(type, "virt_functions")) { > + goto cleanup; > + } else { > + size_t i; > int naddresses; > > if ((naddresses = virXPathNodeSet("./address", ctxt, &addresses)) < > 0) > - goto out; > + goto cleanup; > > if (maxFuncsStr && > virStrToLong_uip(maxFuncsStr, NULL, 10, > &pci_dev->max_virtual_functions) < 0) { > virReportError(VIR_ERR_XML_ERROR, "%s", > _("Malformed 'maxCount' parameter")); > - goto out; > + goto cleanup; > } > > if (VIR_ALLOC_N(pci_dev->virtual_functions, naddresses) < 0) > - goto out; > + goto cleanup; > > for (i = 0; i < naddresses; i++) { > virPCIDeviceAddressPtr addr = NULL; > > if (VIR_ALLOC(addr) < 0) > - goto out; > + goto cleanup; > > if (virPCIDeviceAddressParseXML(addresses[i], addr) < 0) { > VIR_FREE(addr); > - goto out; > + goto cleanup; > } > > if (VIR_APPEND_ELEMENT(pci_dev->virtual_functions, > pci_dev->num_virtual_functions, > addr) < 0) > - goto out; > + goto cleanup; > } > + } > > - pci_dev->flags |= VIR_NODE_DEV_CAP_FLAG_PCI_VIRTUAL_FUNCTION; > + pci_dev->flags |= cap; > + ret = 0; > + cleanup: > + VIR_FREE(addresses); > + VIR_FREE(maxFuncsStr); > + return ret; > +} > + > + > +static int > +virNodeDevPCICapabilityParseXML(xmlXPathContextPtr ctxt, > + xmlNodePtr node, > + virNodeDevCapPCIDevPtr pci_dev) > +{ > + char *type = virXMLPropString(node, "type"); > + xmlNodePtr orignode = ctxt->node; > + unsigned int sriov_cap = 0; > + int ret = -1; > + > + ctxt->node = node; > + > + if (!type) { > + virReportError(VIR_ERR_XML_ERROR, "%s", _("Missing capability > type")); > + goto cleanup; > + } > + > + if (STREQ(type, "phys_function")) > + sriov_cap = VIR_NODE_DEV_CAP_FLAG_PCI_PHYSICAL_FUNCTION; > + else if (STREQ(type, "virt_functions")) > + sriov_cap = VIR_NODE_DEV_CAP_FLAG_PCI_VIRTUAL_FUNCTION; > + > + if (sriov_cap && > + virNodeDevPCICapSRIOVParseXML(ctxt, node, pci_dev, sriov_cap) < 0) { > + goto cleanup; If you drop the sriov_cap the code can be cleaner by simply passing the VIR_NODE_DEV_CAP_FLAG_PCI_* flag directly as parameter and if you decide to split the virNodeDevPCICapSRIOVParseXML() you will not need to pass it at all.) Pavel > } else { > int hdrType = virPCIHeaderTypeFromString(type); > > @@ -1364,9 +1388,7 @@ virNodeDevPCICapabilityParseXML(xmlXPathContextPtr ctxt, > } > > ret = 0; > - out: > - VIR_FREE(addresses); > - VIR_FREE(maxFuncsStr); > + cleanup: > VIR_FREE(type); > ctxt->node = orignode; > return ret; > -- > 2.12.2 > > -- > libvir-list mailing list > libvir-list@redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list