[PATCH virt-install v1 2/3] virt-xml: add support for mediated devices

[Shalini Chellathurai Saroja]

On 5/19/21 12:46 AM, Cole Robinson wrote:
> On 4/14/21 11:18 AM, Shalini Chellathurai Saroja wrote:
>> Provide support to add/remove mdev in a guest domain, which is in
>> shut-off or running state (hotplug/unplug). Also support update of
>> already existing mdev device, when the guest domain is in shut-off
>> state. Please note that libvirt does not support update of mdev
>> device, when the guest domain is in running state.
>>
>> Signed-off-by: Shalini Chellathurai Saroja <shalini at linux.ibm.com>
> Patch looks pretty good, but I have some comments that I think will
> require a v2. See below

Hello Cole,

Thank you. ok, sure.

>
>> diff --git a/tests/utils.py b/tests/utils.py
>> index 802d6590..8a8a243a 100644
>> --- a/tests/utils.py
>> +++ b/tests/utils.py
>> @@ -232,7 +232,7 @@ def diff_compare(actual_out, filename=None, expect_out=None):
>>               open(filename, "w").write(actual_out)
>>           expect_out = open(filename).read()
>>   
>> -    diff = xmlutil.diff(expect_out, actual_out,
>> +    diff = xmlutil.diff(expect_out.rstrip(), actual_out.rstrip(),
>>               filename or '', "Generated output")
> Is this necessary? I can't tell from test output what the point is. If
> so I'd rather have it done as a separate patch so it's easier to see how
> it affects the test suite output in isolation.
yes, it is necessary for my tests. I will move this and the tests to a 
separate patch.
>
>>       if diff:
>>           raise AssertionError("Conversion outputs did not match.\n%s" % diff)
>> diff --git a/virtinst/nodedev.py b/virtinst/nodedev.py
>> index 97841794..873002ec 100644
>> --- a/virtinst/nodedev.py
>> +++ b/virtinst/nodedev.py
>> @@ -5,6 +5,7 @@
>>   # See the COPYING file in the top-level directory.
>>   
>>   import os
>> +import uuid
>>   
>>   from .logger import log
>>   from .xmlbuilder import XMLBuilder, XMLProperty, XMLChildProperty
>> @@ -101,6 +102,12 @@ class NodeDevice(XMLBuilder):
>>                   _compare_int(self.bus, hostdev.bus) and
>>                   _compare_int(self.device, hostdev.device))
>>   
>> +        if self.device_type == "mdev":
>> +            if hostdev.type != "mdev":
>> +                return False
>> +
>> +            return uuid.UUID(self.name[5:].replace('_', '-')) == uuid.UUID(hostdev.uuid)
>> +
> These UUID conversions should be wrapped in try except. See how
> _compare_int handles int conversions as an example. We need to be very
> conservative here because of the way this lookup path is used in places
> like virt-manager IIRC
Sure, I will do it.
>
> Also the self.name[5:]... bit is repeated in several places. It would
> help to add a get_mdev_uuid() function to NodeDevice IMO that
> centralizes this logic
ok.
>
>>           return False
>>   
>>   
>> @@ -207,6 +214,10 @@ def _AddressStringToHostdev(conn, addrstr):
>>               hostdev.type = "usb"
>>               hostdev.bus = bus
>>               hostdev.device = device
>> +
>> +        elif "mdev_" in addrstr:
>> +            hostdev.type = "mdev"
>> +            hostdev.uuid = addrstr[5:].replace('_', '-')
>>           else:
>>               raise RuntimeError("Unknown address type")
>>       except Exception:
>>
> This special syntactic sugar is intended only for --hostdev $PCI and
> --hostdev $USB addresses of the well known address forms. I don't think
> it's necessary for this case, so please drop it or explain what I missed.

Yes, it is not. I will drop this code snippet.

Thank you for the review. I will incorporate these changes and send a v2.

>
> Thanks,
> Cole
>
-- 
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