Hello Erik,

I will make changes based on your comments in here and patch 03. Thank you very much for the review.

On 11/30/20 3:20 PM, Erik Skultety wrote:
On Tue, Nov 17, 2020 at 01:10:55PM +0100, Shalini Chellathurai Saroja wrote:
Introduce support for the Adjunct Processor (AP) crypto card device.
Udev already detects the device, so add support for libvirt nodedev
driver.

Signed-off-by: Farhan Ali <al...@linux.ibm.com>
Signed-off-by: Shalini Chellathurai Saroja <shal...@linux.ibm.com>
Reviewed-by: Bjoern Walk <bw...@linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiu...@linux.ibm.com>
---
  docs/formatnode.html.in            |  8 ++++++
  docs/schemas/nodedev.rng           | 10 ++++++++
  src/conf/node_device_conf.c        | 41 ++++++++++++++++++++++++++++++
  src/conf/node_device_conf.h        |  8 ++++++
  src/conf/virnodedeviceobj.c        |  1 +
  src/node_device/node_device_udev.c | 29 +++++++++++++++++++++
  tools/virsh-nodedev.c              |  1 +
  7 files changed, 98 insertions(+)

...

+static int
+virNodeDevCapAPCardParseXML(xmlXPathContextPtr ctxt,
+                            virNodeDeviceDefPtr def,
+                            xmlNodePtr node,
+                            virNodeDevCapAPCardPtr ap_card)
+{
+    xmlNodePtr orig;
+    g_autofree char *adapter = NULL;
+
+    orig = ctxt->node;
+    ctxt->node = node;
+
+    if (!(adapter = virXPathString("string(./ap-adapter[1])", ctxt))) {
+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                       _("missing ap-adapter value for '%s'"), def->name);
+        return -1;
+    }
+
+    if (virStrToLong_uip(adapter, NULL, 0, &ap_card->ap_adapter) < 0) {
+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                       _("invalid ap-adapter value '%s' for '%s'"),
+                       adapter, def->name);
+        return -1;
+    }
+
+    ctxt->node = orig;
^this restore will not happen if any of the above fails. I'd suggest using the
VIR_XPATH_NODE_AUTORESTORE(ctxt) macro which already adopts our latest g_auto*
standard.

In context of patch 3, I'd also suggest to extract the adapter parsing logic
into another static helper function e.g. virNodeDevAPAdapterParseXML which
could be reused later from virNodeDevCapAPQueueParseXML to avoid duplication.

Erik

--
Kind regards
Shalini Chellathurai Saroja
Linux on Z and Virtualization Development
Vorsitzende des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294


Reply via email to