[Kimchi-devel] [PATCH 2/4] host pci pass through: list all types of host devices

[Zhou Zheng Sheng]

on 2014/04/24/ 16:49, Mark Wu wrote:
> On 04/23/2014 05:48 PM, Zhou Zheng Sheng wrote:
>> +def _xpath(xml_str):
>> +    def get_first_text(the_path):
>> +        r = xpath_get_text(xml_str, the_path)
>> +        if r == []:
>> +            return None
>> +        r = r[0]
>> +        if r is None:
>> +            return ''
>> +        return r
>> +    return get_first_text
> It can serve for general purpose.  So do you think it's better move to
> utils.

Agree. Thanks.

>> +
>> +
>> +def get_all_host_devices(libvirt_conn):
> Any reason to keep an argument for libvirt connection?  I mean you can
> get the connection inside this function.

Nice suggestion, thanks. Generally I used to not use singleton and
global variable in the first draft. I like having the caller insert
dependencies via the parameters, rather than have the callee find the
dependency. This is because if we have two libvirt connections to
different remote hosts, it's better to let the caller to specify which
connection to use. Currently this is not the case so it's OK to use the
global singleton here.

>> +def _get_pci_dev_info(xml_str):
>> +    xpath = _xpath(xml_str)
>> +    info = {
>> +        'domain': xpath('/device/capability/domain'),
>> +        'bus': xpath('/device/capability/bus'),
>> +        'slot': xpath('/device/capability/slot'),
>> +        'function': xpath('/device/capability/function'),
>> +        'product': {
>> +            'id': xpath('/device/capability/product/@id'),
>> +            'description': xpath('/device/capability/product')},
>> +        'vendor': {
>> +            'id': xpath('/device/capability/vendor/@id'),
>> +            'name': xpath('/device/capability/vendor')}}
> the xml path of device info can be categorized into a few patterns, then
> you can use helper functions
> to get device info for all kinds of devices.  It will make the code look
> clean.
> 

Get it. Thanks.

>> +def _get_system_dev_info(xml_str):
>> +    xpath = _xpath(xml_str)
>> +    info = {
>> +        'hardware': {
>> +            'vendor': xpath('/device/capability/hardware/vendor'),
>> +            'version': xpath('/device/capability/hardware/version'),
>> +            'serial': xpath('/device/capability/hardware/serial'),
>> +            'uuid': xpath('/device/capability/hardware/uuid')},
>> +        'firmware': {
>> +            'vendor': xpath('/device/capability/firmware/vendor'),
>> +            'version': xpath('/device/capability/firmware/version'),
>> +            'release_date':
>> xpath('/device/capability/firmware/version')}}
> should be release_date, not version.

Nice catch! Thanks.