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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list