On Fri Mar 14 21:38:17 2025 +0000, Vibhav Pant wrote:
So, the _only_ flag for controlling what kind of inquiry needs to be
performed is `LUP_CONTAINERS`. If it's there, WSALookupServiceBegin will look up Bluetooth devices. If it's not, it'll go through Bluetooth services (on either the local radio(s) or a remote device, determined by `WSAQUERYSET` fields). The documentation doesn't do a great job of explaining this (though the API itself is quite unintuitive to begin with). I've added some documentation to ``sh_lookup_service_begin` explaining this.
So LUP_NOCONTAINERS is ignored?
I might have misunderstood your original question, apologies. What I meant was that `LUP_CONTAINERS` and `LUP_FLUSH_CACHE` are only used for `WSALookupServiceBegin`, and ignored for `WSALookupServiceNext`. That's because the lookup type gets set during `WSALookupServiceBegin`, while `WSALookupServiceNext` only iterates through the results.
Either way, why are we still keeping the flag inside of
ws_bth_query_ctx and not putting it in ws_lookup_service_ctx? The flag informs `WSALookupServiceNext` what kind of lookup results does the handle store, and therefore which union field we should be using to iterate. Once we have support for SDP service lookup, the union would have a `struct ws_bth_query_service` field, and we'd access it if `lookup_devices` was set to false. That way, `lookup_devices` serves as the tag bit for the union, and IMO it's clearer for it to be alongside the union.
Sorry, let me clarify. According to my reading of the documentation alone—which is of course not to be trusted, but also isn't tested in this patch series—the parameters to WSALookupServiceBegin() are only filters. In particular, the flags LUP_CONTAINERS and LUP_NOCONTAINERS both exist. I would *assume* that specifying LUP_CONTAINERS causes only containers to be returned, LUP_NOCONTAINERS causes only services to be returned, and specifying neither flag means that both containers and services will be returned. Possibly this is not actually what happens.
Actually, a similar issue is suggested by the documentation for the first parameter: the dwNameSpace parameter can be NS_ALL, which implies that having any unions at all in struct ws_lookup_service_ctx is problematic.
This is not to say that we need to go as far as testing LUP_NOCONTAINERS or NS_ALL. In fact, I am inclined to suspect this patch goes to excessive lengths to future-proof the code. It's not like putting things in unions after the fact is generally that much of a problem, in my opinion. At the same time, I as a maintainer would not necessarily reject a future-proof design that I feel is sufficiently unobtrusive and/or likely to be necessary. But it's not clear that this design *is* going in the right direction; the documentation suggests it's not and it doesn't have any tests (even external informal ones) to contradict the documentation.