Hi, Was a bit too quick with my updated patch! Anyway... On 2013-07-24 20:38, anderson.lizardo@xxxxxxxxxxxxx wrote: > Yes, but 128-bit == 16 bytes, so it should have been 19... but using > ATT_MAX_VALUE_LEN is better so you don't have to worry about the exact > buffer size anyway (it just needs to fit the data). > > ATT_MAX_VALUE_LEN is 512, but it seems to be the maximum size of a > > value, rather than the UUID. It's more than enough at least :) > The UUID is being saved into an attribute, and attributes have maximum > value size of 512, that's why I proposed using this define instead of > a hardcoded size. Oh, I see your point. I'll update the patch. > > > > - g_assert(h - start_handle == (uint16_t) size); > > > > + g_assert((uint16_t) (h - start_handle) == (uint16_t) size); > > > > > This actually seems unrelated to the patch. Can you put it into a > > > separate patch with the compilation error you got? > > > > When using 128-bit UUIDs, the start handle ultimately comes from > > find_uuid128_avail, which returns 0xffff - nitems + 1 (for the first > > item). If I don't explicitly cast it, h - start_handle will be negative > > number. So, my patch doesn't really work without this. > Ok, the problem is clearer now. "h" is overflowing for 128-bit > services because the handle is incremented after each attribute is > added so at the end we have: 0xffff + 1 == 0 (h is uint16_t). > As it seems more complex to try to remove the overflow without > affecting the logic too much, can you try the following change > instead: > - g_assert(h - start_handle == (uint16_t) size); > + g_assert(h == 0 || (h - start_handle == (uint16_t) size)); > In my opinion, the cast you added is dangerous because it will hide > the programming bugs that this g_assert() was supposed to catch. Sure, will do that, too. > Given this is a bug on the existing code and not part of the > implementation of GATT_OPT_CHR_UUID, please put it into a separate > patch. This means you should send 2 patches, the first with this fix > and the second with the remaining changes. Okidoki. I actually split the original patch in two, so their separately applicable. Cheers, Dirk. -- Dirk-Jan C. Binnema Helsinki, Finland e:djcb@xxxxxxxxxxxxxxx w:www.djcbsoftware.nl pgp: D09C E664 897D 7D39 5047 A178 E96A C7A1 017D DA3C -- To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
