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 <eskultet@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
