[PATCH libvirt v3 01/11] nodedev: detect AP card device

[Shalini Chellathurai Saroja]

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 <alifm at linux.ibm.com>
>> Signed-off-by: Shalini Chellathurai Saroja <shalini at linux.ibm.com>
>> Reviewed-by: Bjoern Walk <bwalk at linux.ibm.com>
>> Reviewed-by: Boris Fiuczynski <fiuczy at 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