[libhid-discuss] Internal definition of struct usb_dev_handle breaks portability

[Albert Lee]

On Thu, 29 Mar 2007, Charles Lepple wrote:

> On Mar 29, 2007, at 8:33 PM, Albert Lee wrote:
>
>> I noticed problems building libhid on Solaris because libhid makes 
>> assumptions about struct usb_dev_handle, which it keeps internal copies of. 
>> This is fairly bad.  Is there a reason not to use usb_device() to get the 
>> device from the handle instead of poking at the struct contents?
>
> I don't know of any reasons offhand, but then again, I think Martin or Arnaud 
> wrote that code (not to pass the buck - it's just that none of us were really 
> doing all that much with Solaris).
>
> Can you post some of the errors?
>
Actually, it compiled and linked (after some build system changes), but 
segfaulted at runtime since the struct contents were different from the 
private copy's. libusb on Solaris doesn't actually expose the definition 
of usb_dev_handle, but the interface accepts pointers to it. In the 
attached patch I have changed the code to just use struct usb_device.

-Albert
-------------- next part --------------
Index: test/lshid.c
===================================================================
--- test/lshid.c	(revision 318)
+++ test/lshid.c	(working copy)
@@ -27,29 +27,22 @@
 
 char *hid_id[32]; /* FIXME: 32 devices MAX */
 
-/* NOTE: included from libusb/usbi.h. UGLY, i know, but so is libusb! */
-struct usb_dev_handle {
-  int fd;
-  struct usb_bus *bus;
-  struct usb_device *device;
-  int config;
-  int interface;
-  int altsetting;
-  void *impl_info;
-};
+struct usb_dev_handle;
 
 bool device_iterator (struct usb_dev_handle const* usbdev, void* custom, unsigned int len)
 {
   bool ret = false;
   int i;
   char current_dev_path[10];
+  const struct usb_device *device = usb_device((struct usb_dev_handle *)usbdev);
   
   /* only here to prevent the unused warning */
   /* TODO remove */
   len = *((unsigned long*)custom);
  
   /* Obtain the device's full path */
-  sprintf(current_dev_path, "%s/%s", usbdev->bus->dirname, usbdev->device->filename);
+  //sprintf(current_dev_path, "%s/%s", usbdev->bus->dirname, usbdev->device->filename);
+  sprintf(current_dev_path, "%s/%s", device->bus->dirname, device->filename);
 
   /* Check if we already saw this dev */
   for ( i = 0 ; ( hid_id[i] != NULL ) ; i++ )
@@ -61,8 +54,8 @@
   /* Append device to the list if needed */
   if (hid_id[i] == NULL)
 	{
-	  hid_id[i] = (char *) malloc (strlen(usbdev->device->filename) + strlen(usbdev->bus->dirname) );
-	  sprintf(hid_id[i], "%s/%s", usbdev->bus->dirname, usbdev->device->filename);
+	  hid_id[i] = (char *) malloc (strlen(device->filename) + strlen(device->bus->dirname) );
+	  sprintf(hid_id[i], "%s/%s", device->bus->dirname, device->filename);
 	}
   else /* device already seen */
 	{
@@ -70,9 +63,9 @@
 	}
 
   /* Filter non HID device */
-  if ( (usbdev->device->descriptor.bDeviceClass == 0) /* Class defined at interface level */
-	&& usbdev->device->config
-	&& usbdev->device->config->interface->altsetting->bInterfaceClass == USB_CLASS_HID)
+  if ( (device->descriptor.bDeviceClass == 0) /* Class defined at interface level */
+	&& device->config
+	&& device->config->interface->altsetting->bInterfaceClass == USB_CLASS_HID)
 	  ret = true;
   else
 	  ret = false;
Index: m4/md_check_os.m4
===================================================================
--- m4/md_check_os.m4	(revision 318)
+++ m4/md_check_os.m4	(working copy)
@@ -15,6 +15,12 @@
         MD_OS=bsd
         AC_MSG_RESULT(*BSD)
         ;;
+      *-solaris*)
+        AC_DEFINE(OS_SOLARIS, [], [define to 1 if building for Solaris])
+        AC_SUBST(OS_SOLARIS)
+        MD_OS=solaris
+        AC_MSG_RESULT(Solaris)
+	;;
       *-darwin*)
         AC_DEFINE(OS_DARWIN, [], [define to 1 if building for OS X (Darwin)])
         AC_SUBST(OS_DARWIN)
Index: src/debug.c
===================================================================
--- src/debug.c	(revision 318)
+++ src/debug.c	(working copy)
@@ -21,16 +21,7 @@
   usb_set_debug(level);
 }
 
-/* NOTE: included from libusb/usbi.h. UGLY, i know, but so is libusb! */
-struct usb_dev_handle {
-  int fd;
-  struct usb_bus *bus;
-  struct usb_device *device;
-  int config;
-  int interface;
-  int altsetting;
-  void *impl_info;
-};
+struct usb_dev_handle;
 
 void trace_usb_bus(FILE* out, struct usb_bus const* usbbus)
 {
@@ -87,14 +78,8 @@
 
 void trace_usb_dev_handle(FILE* out, usb_dev_handle const* usbdev_h)
 {
-  fprintf(out, "usb_dev_handle instance at: %10p\n", usbdev_h);
-  fprintf(out, "  fd:                       %d\n", usbdev_h->fd);
-  fprintf(out, "  bus:                      %10p\n", usbdev_h->bus);
-  fprintf(out, "  device:                   %10p\n", usbdev_h->device);
-  fprintf(out, "  config:                   %d\n", usbdev_h->config);
-  fprintf(out, "  interface:                %d\n", usbdev_h->interface);
-  fprintf(out, "  altsetting:               %d\n", usbdev_h->altsetting);
-  fprintf(out, "  impl_info:                %10p\n", usbdev_h->impl_info);
+  struct usb_device *device = usb_device((usb_dev_handle *)usbdev_h);
+  trace_usb_device(out, device);
 }
 
 /* COPYRIGHT --
Index: src/Makefile.am
===================================================================
--- src/Makefile.am	(revision 318)
+++ src/Makefile.am	(working copy)
@@ -11,6 +11,9 @@
 if OS_BSD
 OS_SUPPORT_SOURCE = bsd.c
 else
+if OS_SOLARIS
+OS_SUPPORT_SOURCE = bsd.c
+else
 if OS_DARWIN
 OS_SUPPORT_SOURCE = darwin.c
 AM_CFLAGS += -no-cpp-precomp
@@ -20,6 +23,7 @@
 endif
 endif
 endif
+endif
 
 lib_LTLIBRARIES = libhid.la
 libhid_la_DEPENDENCIES = ../hidparser/libhidparser.la
Index: configure.ac
===================================================================
--- configure.ac	(revision 318)
+++ configure.ac	(working copy)
@@ -15,7 +15,7 @@
 m4_define([LIBHID_VERSION], [0.2.16])
 m4_define([IFACE_AGE], [0])
 m4_define([BIN_AGE],   [0])
-m4_define([MD_INIT_NAME], m4_esyscmd([set -- `head -1 README` && echo -n $1]))
+m4_define([MD_INIT_NAME], m4_esyscmd([set -- `head -1 README` && echo $1 | tr -d "\n"]))
 m4_define([MD_INIT_VERSION], LIBHID_VERSION.IFACE_AGE.BIN_AGE)
 m4_define([MD_INIT_ADDRESS], m4_esyscmd([grep -i bugreports\ to README | sed -e 's,.*<\([^>]*\).*,\1,' | tr -d "\n"]))
 
@@ -79,6 +79,7 @@
 MD_CONF_COMPILER
 AM_CONDITIONAL(OS_LINUX, test "$MD_OS" = "linux")
 AM_CONDITIONAL(OS_BSD, test "$MD_OS" = "bsd")
+AM_CONDITIONAL(OS_SOLARIS, test "$MD_OS" = "solaris")
 AM_CONDITIONAL(OS_DARWIN, test "$MD_OS" = "darwin")
 
 MD_CHECK_SWIG([1.3])