On Wed, Oct 02, 2024 at 01:15:00PM +0100, John Levon wrote:
On Wed, Oct 02, 2024 at 01:00:33PM +0200, Martin Kletzander wrote:> static int > testDomainAttachHostDevice(testDriver *driver, > virDomainObj *vm, > @@ -10144,28 +10232,68 @@ testDomainAttachDeviceLive(virDomainObj *vm, > testDriver *driver) > { > const char *alias = NULL; > + int ret = -1; > > - if (dev->type != VIR_DOMAIN_DEVICE_HOSTDEV) { > + switch (dev->type) { > + case VIR_DOMAIN_DEVICE_NET: > + testDomainObjCheckNetTaint(vm, dev->data.net); > + ret = testDomainAttachNetDevice(driver, vm, dev->data.net); > + if (!ret) { if this is an integer where 0 is success and negative number is an error we tend to check for (ret == 0). In this case it does not do much, but in other instances it allows us to add different success return values in the future. And keeping the same way of doing things makes it nicer as consistent appearance is easier to read.Isn't it better to be consistent with the original code? This came directly from qemu_hotplug.c
Well, I could say that qemu's hotplug is a bit more complex as it uses the @ret variable later on to decide whether to update the device list. But this particular function in qemu_hotplug.c is very similar, only a bit ugly, precisely because it uses mixture of (!ret) and (ret == 0).
> + alias = dev->data.hostdev->info->alias; > + dev->data.hostdev = NULL; And since these two lines are the same for both code paths you can do that after the switch when you error out early ...ditto, I think?
This one is my fault, I did not notice this accessing a different member (duh!) so this needs to stay as it is anyway. If you are fine with me removing the audit call, audit header include and changing the "!ret" to "ret == 0", I'll fixup those changes and push it as I'm fine with these patches given that modifications. Let me know. Have a nice day, Martin
regards john
Attachment:
signature.asc
Description: PGP signature
