On 02/27/2014 02:27 PM, Michal Privoznik wrote:
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.
This pattern is used by virportallocatortests, which I touched last :) In both cases, I think the test should only be run on Linux, since Eric showed that #if HAVE_DLFCN_H is not enough for Cygwin and most of the virusb.c file is Linux-specific.
+ 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.
Yes, that might look nicer.
+ } 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
I was hoping to dig out the patch adding support for specifying USB devices by serials someday, but they don't really belong in this patch. Thanks, Jan