ng_hci: Cast NG_HCI_BDADDR_ANY to const bdaddr_t *

[christos]

Summary

This is needed to address some compiler errors cleanly, where consumer
functions want this address to be a const pointer.

Sponsored by: The FreeBSD Foundation
MFC after: 3 days

[christos]

Not 100% sure if that's a correct fix.

[christos]

This is needed for D52308 to build.

[kevans]

sys/netgraph/bluetooth/include/ng_hci.h

452

What breaks if you make it a const bdaddr_p instead? We probably really don't want anything mutating it.

[christos]

sys/netgraph/bluetooth/include/ng_hci.h

452

With this change it still compiles:

diff --git a/sys/netgraph/bluetooth/include/ng_hci.h b/sys/netgraph/bluetooth/include/ng_hci.h
index 19088d3ab13f..1ecc75c5affe 100644
--- a/sys/netgraph/bluetooth/include/ng_hci.h
+++ b/sys/netgraph/bluetooth/include/ng_hci.h
@@ -449,7 +449,7 @@ typedef bdaddr_t *  bdaddr_p;

 /* Any BD_ADDR. Note: This is actually 7 bytes (count '\0' terminator) */
 #define NG_HCI_BDADDR_ANY      \
-       ((bdaddr_p) __DECONST(char *, "\000\000\000\000\000\000"))
+       (__DECONST(const bdaddr_p, "\000\000\000\000\000\000"))

[markj]

sys/netgraph/bluetooth/include/ng_hci.h

452

And what if you remove the __DECONST from that?

[christos]

sys/netgraph/bluetooth/include/ng_hci.h

452

Without the __DECONST I get the same error as in the commit message.

[markj]

sys/netgraph/bluetooth/include/ng_hci.h

452

Ok, so we want NG_HCI_BDADDR_ANY to be a pointer to const. Note that const bdaddr_p is a const pointer since bdaddr_p is a pointer type. So neither proposed patch is really right.

I'd suggest defining it this way instead: (&(const bdaddr_t){}). Maybe some of the consumers are not const-clean (e.g., sdp_register_service() looks wrong), those should be fixed (or __DECONST should be applied there) instead of mangling this definition.

[christos]

sys/netgraph/bluetooth/include/ng_hci.h

452

Do you agree with defining:

/* Any BD_ADDR. Note: This is actually 7 bytes (count '\0' terminator) */
#define NG_HCI_BDADDR_ANY	(&(const bdaddr_t){"\000\000\000\000\000\000"})

as you said, which probably should be that way in the first place, and then simply __DECONSTing in the sdp_register_service() call in virtual_bt_speaker(8)?

	if (sdp_register_service(r->sdp_session, SDP_SERVICE_CLASS_AUDIO_SINK,
	    __DECONST(bdaddr_p, NG_HCI_BDADDR_ANY),
	    (const uint8_t *)&record, sizeof(record),
	    &r->sdp_handle)) {

There are a few more calls to this function in usr.bin/bluetooth/{rfcomm_sppd/rfcomm_sppd.c, btpand/server.c, rfcomm_pppd/rfcomm_pppd}.

[markj]

sys/netgraph/bluetooth/include/ng_hci.h

452

I think that's ok. Or see if you can make sdp_register_service() take a pointer to const itself, and use __DECONST internally.

[imp]

sys/netgraph/bluetooth/include/ng_hci.h

452

Why not have an btaddr_any global thats of the right type, initialized to 0? The the whole DECONST jank goes away, the weird cast goes away, etc. What is wrong with that more straight forward approach?

[markj]

sys/netgraph/bluetooth/include/ng_hci.h

452

If that global is const, then I don't see how the DECONST jank goes away. If it's not const, then it's susceptible to accidental modification.

There is some precedent for your suggestion in in6addr_any and so on, but it requires that a symbol be defined somewhere. I don't see why it's better than the other proposals?

[christos]

@markj git-arc(1) for some reason doesn't let me update the diff (maybe the title is the issue?), so here is the new diff:

diff --git a/sys/netgraph/bluetooth/include/ng_hci.h b/sys/netgraph/bluetooth/include/ng_hci.h
index 44a14e62f4ed..ce3291770740 100644
--- a/sys/netgraph/bluetooth/include/ng_hci.h
+++ b/sys/netgraph/bluetooth/include/ng_hci.h
@@ -448,7 +448,7 @@ typedef struct {
 typedef bdaddr_t *     bdaddr_p;

 /* Any BD_ADDR. Note: This is actually 7 bytes (count '\0' terminator) */
-#define NG_HCI_BDADDR_ANY      ((bdaddr_p) "\000\000\000\000\000\000")
+#define NG_HCI_BDADDR_ANY      (&(const bdaddr_t){"\000\000\000\000\000\000"})

 /* HCI status return parameter */
 typedef struct {

[christos]

In D52310#1198802, @christos wrote:

@markj git-arc(1) for some reason doesn't let me update the diff (maybe the title is the issue?), so here is the new diff:

diff --git a/sys/netgraph/bluetooth/include/ng_hci.h b/sys/netgraph/bluetooth/include/ng_hci.h
index 44a14e62f4ed..ce3291770740 100644
--- a/sys/netgraph/bluetooth/include/ng_hci.h
+++ b/sys/netgraph/bluetooth/include/ng_hci.h
@@ -448,7 +448,7 @@ typedef struct {
 typedef bdaddr_t *     bdaddr_p;

 /* Any BD_ADDR. Note: This is actually 7 bytes (count '\0' terminator) */
-#define NG_HCI_BDADDR_ANY      ((bdaddr_p) "\000\000\000\000\000\000")
+#define NG_HCI_BDADDR_ANY      (&(const bdaddr_t){"\000\000\000\000\000\000"})

 /* HCI status return parameter */
 typedef struct {

Bump.

[markj]

In D52310#1198802, @christos wrote:

@markj git-arc(1) for some reason doesn't let me update the diff (maybe the title is the issue?), so here is the new diff:

The commit and review titles have to match.

diff --git a/sys/netgraph/bluetooth/include/ng_hci.h b/sys/netgraph/bluetooth/include/ng_hci.h
index 44a14e62f4ed..ce3291770740 100644
--- a/sys/netgraph/bluetooth/include/ng_hci.h
+++ b/sys/netgraph/bluetooth/include/ng_hci.h
@@ -448,7 +448,7 @@ typedef struct {
 typedef bdaddr_t *     bdaddr_p;

 /* Any BD_ADDR. Note: This is actually 7 bytes (count '\0' terminator) */
-#define NG_HCI_BDADDR_ANY      ((bdaddr_p) "\000\000\000\000\000\000")
+#define NG_HCI_BDADDR_ANY      (&(const bdaddr_t){"\000\000\000\000\000\000"})

 /* HCI status return parameter */
 typedef struct {

Ok.