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
> > + 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?
regards
john
