Re: [libvirt] [PATCH 1/2] Add tests for virUSBDeviceFind functions

On 26.02.2014 17:54, Ján Tomko wrote: > Mock the /sys/bus/usb directory and test the finding > (and not finding) of some USB devices. > ---

> @@ -815,6 +817,16 @@ virpcimock_la_LIBADD = $(GNULIB_LIBS) \ > virpcimock_la_LDFLAGS = -module -avoid-version \ > -rpath /evil/libtool/hack/to/force/shared/lib/creation > > +virusbtest_SOURCES = \ > + virusbtest.c testutils.h testutils.c > +virusbtest_LDADD = $(LDADDS) > + > +virusbmock_la_SOURCES = \ > + virusbtest.c > +virusbmock_la_CFLAGS = $(AM_CFLAGS) -DMOCK_HELPER=1 > +virusbmock_la_LDFLAGS = -module -avoid-version \ > + -rpath /evil/libtool/hack/to/force/shared/lib/creation Is there any specific reason why the mock functions are not in a separate file? We have that pattern already.

> + > if WITH_DBUS > virdbustest_SOURCES = \ > virdbustest.c testutils.h testutils.c > diff --git a/tests/virusbtest.c b/tests/virusbtest.c > new file mode 100644 > index 0000000..f9104bf > --- /dev/null > +++ b/tests/virusbtest.c > +static int testDeviceFind(const void *opaque) > +{ > + const struct findTestInfo *info = opaque; > + int ret = -1; > + virUSBDevicePtr dev = NULL; > + virUSBDeviceListPtr devs = NULL; > + int rv = 0; > + size_t i, ndevs = 0; > + > + switch (info->how) { > + case FIND_BY_ALL: > + rv = virUSBDeviceFind(info->vendor, info->product, > + info->bus, info->devno, > + info->vroot, info->mandatory, &dev); > + break; > + case FIND_BY_VENDOR: > + rv = virUSBDeviceFindByVendor(info->vendor, info->product, > + info->vroot, info->mandatory, &devs); > + break; > + case FIND_BY_BUS: > + rv = virUSBDeviceFindByBus(info->bus, info->devno, > + info->vroot, info->mandatory, &dev); > + break; > + } > + > + if (info->expectFailure) { > + if (rv == 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + "unexpected success"); > + goto cleanup; > + } Would it make sense to: ret = 0; goto cleanup; since tesing either an empty device list or NULL dev doesn't make much sense? I know you're calling FileIterate only when there's something to call it on, but sill.

> + } else if (rv < 0) { > + goto cleanup; > + } > + > + if (dev && virUSBDeviceFileIterate(dev, testDeviceFileActor, NULL) < 0) > + goto cleanup; > + > + if (devs) > + ndevs = virUSBDeviceListCount(devs); > + > + for (i = 0; i < ndevs; i++) { > + virUSBDevicePtr device = virUSBDeviceListGet(devs, i); > + if (virUSBDeviceFileIterate(device, testDeviceFileActor, NULL) < 0) > + goto cleanup; > + } > + > + ret = 0; > + > +cleanup: > + virObjectUnref(devs); > + virUSBDeviceFree(dev); > + return ret; > +} > +

> +++ b/tests/virusbtestdata/sys_bus_usb/devices/1-1.5.3.1/serial > @@ -0,0 +1 @@ > + Why are you creating these /serial files? They don't seem to be used anywhere, moreover, they seem to not exists even in real sysfs (at least on my system): # find /sys/bus/usb/ -name serial | wc -l 0