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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]