On Fri, 2010-02-19 at 19:57 +0100, Hendrik Sattler wrote:
> Hi Bastien,
>
> I have some comments for your patches.
<snip>
> > + while ((usbfd = usbfds[i++]) != NULL) {
> > + if (usbfd->events & POLLOUT)
>
> This is not correct. The file descriptor is used in
> obex_transport_handle_input() in context of the select() funcion. But that
> only waits for incoming data, not for outgoing. So you are choosing the wrong
> file descriptor, here, use the one for POLLIN instead.
You're right. It was a bug in the code I used on top of it that created
the error.
> > + return usbfd->fd;
> > + }
> > +
> > + return INVALID_SOCKET;
> > +}
> > +
> > +/*
> > * Function usbobex_connect_request (self)
> > *
> > * Open the USB connection
> > @@ -379,6 +409,7 @@ int usbobex_connect_request(obex_t *self)
> > }
> >
> > self->trans.mtu = OBEX_MAXIMUM_MTU;
> > + self->fd = usbobex_get_fd();
> > DEBUG(2, "transport mtu=%d\n", self->trans.mtu);
> > return 1;
>
> Additionally to setting self->fd here, you should also modify
> obex_transport_handle_input() to no choose the special USB path when
> self->fd is valid.
Except that it doesn't work because it doesn't actually select on
self->fd, but on self->fd+1.
ret = select((int)highestfd+1, &fdset, NULL, NULL, &time);
I'm pretty sure that's not correct for our case.
> > From: Bastien Nocera <had...@hadess.net>
> > Date: Fri, 19 Feb 2010 13:40:27 +0000
> > Subject: [PATCH] Fix invalid memory access
> >
> > The hdr pointer will not be valid any more if the transport
> > read() does a realloc, so cache the opcode that we'll use
> > later in the code.
>
> Not the correct approach, in my eyes. Instead, update msg (also used after
> this point) and hdr variables.
> Good finding :)
msg wouldn't change. It's a pointer to a struct which contains a pointer
to the actual data. Only ->data would change, not the msg pointer
itself. Fixed in the patch.
Cheers
>From 80ac78ca813d057b2c506f936e1b8b8d3c939357 Mon Sep 17 00:00:00 2001
From: Bastien Nocera <had...@hadess.net>
Date: Fri, 19 Feb 2010 12:03:01 +0000
Subject: [PATCH 1/3] Fix libusb1 detection
There's no need to check for the presence of a .pc file in
/usr/lib/, pkg-config will already do that for you, and it would
fail on machines where /usr/lib64 is used, or when libusb1 is
installed in another prefix.
---
acinclude.m4 | 1 -
1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/acinclude.m4 b/acinclude.m4
index 81fa9da..d813b71 100644
--- a/acinclude.m4
+++ b/acinclude.m4
@@ -191,7 +191,6 @@ AC_DEFUN([AC_PATH_USB], [
AC_DEFUN([AC_PATH_USB1], [
usb1_lib_found=no
PKG_CHECK_MODULES(USB1, libusb-1.0, usb1_lib_found=yes, usb1_lib_found=no)
- AC_CHECK_FILE(${prefix}/lib/pkgconfig/libusb-1.0.pc, REQUIRES="libusb1")
AC_SUBST(USB1_CFLAGS)
AC_SUBST(USB1_LIBS)
--
1.6.6.1
>From 947b313340293c41b9751b1ed5a5caa023c195e9 Mon Sep 17 00:00:00 2001
From: Bastien Nocera <had...@hadess.net>
Date: Fri, 19 Feb 2010 12:04:43 +0000
Subject: [PATCH 2/3] Export the libusb1 read file descriptor
When using libusb1, we can export the file descriptor that
corresponds to reading from the device, so it can be used to
setup polling sources, and timeout based operations.
This would be used by osso-gwobex to monitor incoming data, for
USB support.
---
lib/usb1obex.c | 31 +++++++++++++++++++++++++++++++
1 files changed, 31 insertions(+), 0 deletions(-)
diff --git a/lib/usb1obex.c b/lib/usb1obex.c
index dfa1685..75146a8 100644
--- a/lib/usb1obex.c
+++ b/lib/usb1obex.c
@@ -30,6 +30,7 @@
#include <stdio.h> /* perror */
#include <errno.h> /* errno and EADDRNOTAVAIL */
#include <stdlib.h>
+#include <poll.h> /* POLLIN */
#include <libusb.h>
@@ -339,6 +340,35 @@ void usbobex_free_interfaces(int num, obex_interface_t *intf)
}
/*
+ * Function usbobex_get_fd ()
+ *
+ * Get the "poll out" file descriptor for the USB device,
+ * used to check for events in an async way
+ *
+ */
+static int usbobex_get_fd(void)
+{
+ const struct libusb_pollfd **usbfds;
+ const struct libusb_pollfd *usbfd;
+ int i = 0;
+
+ DEBUG(4, "Getting the USB file descriptor");
+
+ usbfds = libusb_get_pollfds(libusb_ctx);
+ if (usbfds == NULL) {
+ DEBUG(4, "Could not get USB file descriptors");
+ return INVALID_SOCKET;
+ }
+
+ while ((usbfd = usbfds[i++]) != NULL) {
+ if (usbfd->events & POLLIN)
+ return usbfd->fd;
+ }
+
+ return INVALID_SOCKET;
+}
+
+/*
* Function usbobex_connect_request (self)
*
* Open the USB connection
@@ -379,6 +409,7 @@ int usbobex_connect_request(obex_t *self)
}
self->trans.mtu = OBEX_MAXIMUM_MTU;
+ self->fd = usbobex_get_fd();
DEBUG(2, "transport mtu=%d\n", self->trans.mtu);
return 1;
--
1.6.6.1
>From 88081e8f181d6a6ab7221cdb998970ccf146d839 Mon Sep 17 00:00:00 2001
From: Bastien Nocera <had...@hadess.net>
Date: Fri, 19 Feb 2010 13:40:27 +0000
Subject: [PATCH 3/3] Fix invalid memory access
The hdr pointer will not be valid any more if the transport
read() does a realloc, so reset it after a read().
Fixes the following valgrind error:
==31644== Thread 2:
==31644== Invalid read of size 1
==31644== at 0x4E3D787: obex_data_indication (obex_main.c:307)
==31644== by 0x4E3F403: obex_transport_handle_input (obex_transport.c:72)
==31644== by 0x4E3C67E: OBEX_HandleInput (obex.c:449)
==31644== by 0x4C335BD: gw_obex_request_sync (obex-priv.c:108)
==31644== by 0x4C34114: gw_obex_get (obex-priv.c:939)
==31644== by 0x4C319EE: gw_obex_read_dir (gw-obex.c:198)
==31644== by 0x408222: _retrieve_folder_listing (gvfsbackendobexftp.c:552)
==31644== by 0x408DD1: do_enumerate (gvfsbackendobexftp.c:1549)
==31644== by 0x411491: g_vfs_job_run (gvfsjob.c:198)
==31644== by 0x3C758658CA: ??? (in /lib64/libglib-2.0.so.0.2303.0)
==31644== by 0x3C75863A03: ??? (in /lib64/libglib-2.0.so.0.2303.0)
==31644== by 0x3C74806CA9: start_thread (in /lib64/libpthread-2.11.90.so)
==31644== Address 0x5313f50 is 0 bytes inside a block of size 71,679 free'd
==31644== at 0x4A05255: realloc (vg_replace_malloc.c:476)
==31644== by 0x4E41D12: buf_resize (databuffer.c:147)
==31644== by 0x4E42063: buf_reserve_end (databuffer.c:217)
==31644== by 0x4E3FE90: obex_transport_read (obex_transport.c:519)
==31644== by 0x4E3D711: obex_data_indication (obex_main.c:268)
==31644== by 0x4E3F403: obex_transport_handle_input (obex_transport.c:72)
==31644== by 0x4E3C67E: OBEX_HandleInput (obex.c:449)
==31644== by 0x4C335BD: gw_obex_request_sync (obex-priv.c:108)
==31644== by 0x4C34114: gw_obex_get (obex-priv.c:939)
==31644== by 0x4C319EE: gw_obex_read_dir (gw-obex.c:198)
==31644== by 0x408222: _retrieve_folder_listing (gvfsbackendobexftp.c:552)
==31644== by 0x408DD1: do_enumerate (gvfsbackendobexftp.c:1549)
==31644==
---
lib/obex_main.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/lib/obex_main.c b/lib/obex_main.c
index 9fb65d7..c2d5436 100644
--- a/lib/obex_main.c
+++ b/lib/obex_main.c
@@ -267,6 +267,8 @@ int obex_data_indication(obex_t *self, uint8_t *buf, int buflen)
actual = obex_transport_read(self, size - msg->data_size, buf,
buflen);
+ /* hdr might not be valid anymore if the _read did a realloc */
+ hdr = (obex_common_hdr_t *) msg->data;
/* Check if we are still connected */
/* do not error if the data is from non-empty but partial buffer (custom transport) */
--
1.6.6.1
------------------------------------------------------------------------------
Download Intel® Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs
proactively, and fine-tune applications for parallel performance.
See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev
_______________________________________________
Openobex-users mailing list
Openobex-users@lists.sourceforge.net
http://lists.sourceforge.net/lists/listinfo/openobex-users
