Hi and thanks for you comments Johan, > -----Original Message----- > From: Johan Hedberg [mailto:johan.hedberg@xxxxxxxxx] > Sent: den 15 augusti 2012 12:39 > To: Par-Gunnar HJALMDAHL > Cc: linux-bluetooth@xxxxxxxxxxxxxxx; Anurag GUPTA-1 > Subject: Re: [PATCH] network: Check full BNEP UUID > > There are a couple of things that bug me a bit with this patch. One is > the re-definition of bluetooth_base_uuid which really should only exist > in one central place (lib/uuid.c). Another is the way the code logic > goes. It seems you want to test for two things: > > 1) That the UUID is a Bluetooth UUID. This is the memcmp > with > the last 12 bytes of the Bluetooth base UUID for the UUID128 > case. 16 and 32 bit values are already implicitly assumed to > be > Bluetooth UUIDs as they don't contain any information to > reveal > this as such. > > 2) That the (now determined) Bluetooth UUID has a value less > than or equal to 0xffff. This is quite awkwardly tested for > by > comparing with the two first bytes from bluetooth_base_uuid, > which are both zeros. Based on your code comment it sounds > like > this test should really be covered by bnep_setup_chk(). > > If you really want to have the "<= 0xffff" test be in bnep_setup_decode > then I'd propose something like the attached patch. However, testing > for the exact value of the Bluetooth UUID (once the UUID has been > determined to be a Bluetooth UUID) seems like the task of > bnep_setup_chk from the way you've laid out the functions. > > Johan Basically I tried to keep the patch to a minimum since it was quite a simple issue to fix. I have no problems to use the UUID in lib/uuid.c. I could add a 2 new exported functions for retrieving base UUID, one for little endian and one for big endian, since uuid.c endian depends on CPU endian while PAN endian is always big endian. So: void get_le_bt_base_uuid(uint128_t *uuid); void get_be_bt_base_uuid(uint128_t *uuid); Your suggestion looks fine. It's a bit more effective than my original code. The reason I did the check in bnep_setup_decode() rather than in bnep_setup_chk is that only 16 bits are kept when exiting the function (bnep_setup_decode is the only function aware of the UUID size) and I thought that was a quite straightforward design. If it's OK with you I can create a new patch with suggestion above plus your patch. /P-G -- 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
