2008/4/1, Hendrik Sattler <[EMAIL PROTECTED]>: > I'll comment the patch thus here it comes inlined in parts...
Thanks for your comments. I've attached an updated version of the
patch. I write in C only occasionally, so my code is never perfect on
first try :)
> +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)
Role is actually a bit mask, and only bit 0 is currently defined. I've
changed the comment to explain this. I've also added a flag to this
struct to indicate if the UUID is a default one or not (the default
value of it is defined in the spec).
> + /* 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...
No, there are no defaults if this descriptor is absent. Making it NULL
is the easiest way to indicate absence - I've changed the comment
here.
> + * 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.
I'm only using the types provided by libusb for this data. They are
also used in other already present functions in usbobex.c, but not
exposed to the API.
> + int i;
>
> Not needed, see below.
Removed.
> + 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).
Fixed.
> + 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...
Yeah, and with the flag I added to obex_usb_intf_service_t you can no
longer use the sizeof. I left the magic in place, with an explanation
what it is.
> + DEBUG(2, "Invalid service id descriptor");
> + else if (*service == NULL)
> + *service =
> malloc(sizeof(obex_usb_intf_service_t));
>
> = malloc(sizeof(**service));
> would be better.
Such construct is not used anywhere else in openobex though.
> + if (*service != NULL) {
>
> Wrong indentation of the "else if" above is missing the curly braces.
Thanks, fixed.
> + (*service)->role = buffer[3];
>
> What if that is not 0 or 1?
See above; it's actually a bit mask.
> + 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.
Fixed both.
> +next_desc:
> + buflen -= buffer[0];
> + buffer += buffer[0];
>
> OTOH, if buflen is of type size_t, you need a check here. Still...
Erm, I'm not sure which check - to me it seems robust, since buffer is
unsigned char.
--
Alexander
obex-usb-service-descriptor-1.patch
Description: Binary data
------------------------------------------------------------------------- This SF.net email is sponsored by the 2008 JavaOne(SM) Conference Don't miss this year's exciting event. There's still time to save $100. Use priority code J8TL2D2. http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone
_______________________________________________ Openobex-users mailing list [email protected] http://lists.sourceforge.net/lists/listinfo/openobex-users
