Am Dienstag 01 April 2008 schrieb Alex Kanavin:
> So here's the patch. Like I said, I don't have any hardware to test it
> with (and I'm not aware if any such hardware exists), but the patch
> seems pretty safe to me.
I'll comment the patch thus here it comes inlined in parts...
+typedef struct {
+ /* Role: 0 for server, 1 for client */
+ uint8_t role;
+ /* Service UUID */
+ uint8_t uuid[16];
+ /* Service version */
+ uint16_t version;
+} obex_usb_intf_service_t;
Several things:
* role should match the values for OBEX_MODE_(CLIENT|SERVER), yours is inverted
OTOH, that would not meet the value in the spec but a consistent API looks
better.
* make that struct packed and put role at the end (see other comment below)
+ /* Service information, may be NULL */
+ obex_usb_intf_service_t *service;
Why can this be zero? Does the updated standard propose values if that
descriptor is not present? Application code could be simpler if default
values are filled in, I assume...
+ * Helper function to usbobex_find_interfaces
+ */
+static void find_obex_service_descriptor(unsigned char *buffer, int buflen,
obex_usb_intf_service_t **service)
+{
Since you used uint8_t for uuid field, why not for buffer? A len argument
should be of type size_t.
+ int i;
Not needed, see below.
+ if (!buffer) {
+ DEBUG(2,"Weird descriptor references");
+ return ;
+ }
+ while (buflen > 0) {
+ if (buffer [1] != USB_DT_CS_INTERFACE) {
+ DEBUG(2,"skipping garbage");
+ goto next_desc;
+ }
+ switch (buffer [2]) {
Please omit the extra space (2 times) but put it into the DEBUG statement
instead
(after the comma, 2 times).
+ case CDC_OBEX_SERVICE_ID_TYPE: /* we've found it */
+ if (buflen < 22)
This is yet another magic number. If obex_usb_intf_service_t is packed, you can
use
3+sizeof(obex_usb_intf_service_t) instead of 22.
A debateable statement of mine, though...
+ DEBUG(2, "Invalid service id descriptor");
+ else if (*service == NULL)
+ *service =
malloc(sizeof(obex_usb_intf_service_t));
= malloc(sizeof(**service));
would be better.
+ if (*service != NULL) {
Wrong indentation of the "else if" above is missing the curly braces.
+ (*service)->role = buffer[3];
What if that is not 0 or 1?
+ for (i=0; i<16; i++)
+ (*service)->uuid[i] =
buffer[4+i];
memcpy() comes to my mind as replacement for such loops:
memcpy((*service)->uuid, buffer+4, 16)
+ (*service)->version =
buffer[20]*256+buffer[21];
= (buffer[20] << 8) | buffer[21]
is a more common way of writing that.
+next_desc:
+ buflen -= buffer[0];
+ buffer += buffer[0];
OTOH, if buflen is of type size_t, you need a check here. Still...
Have fun.
HS
-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
_______________________________________________
Openobex-users mailing list
Openobex-users@lists.sourceforge.net
http://lists.sourceforge.net/lists/listinfo/openobex-users
