gnrc_netif: add fetch-address and fetch-netif-flag functionalities. #6069

zhuoshuguo · 2016-11-07T17:21:04Z

This PR introduces functionalities that enable fetching source, destination addresses, and netif header flags from netif header of a gnrc packet,

zhuoshuguo · 2016-11-16T19:21:42Z

@miri64 Ping~

zhuoshuguo · 2016-11-20T15:37:54Z

Updated and added unitest codes.

zhuoshuguo · 2016-11-22T08:53:24Z

Reflected module name "gnrc_mac" into internal functions.

zhuoshuguo · 2016-11-29T16:43:05Z

Ping~ ;-)

miri64

Redoing the comments I already made in #6074

miri64 · 2016-11-29T17:04:32Z

sys/include/net/gnrc/mac/internal.h

+ *
+ * @return                          length of destination address
+ */
+int gnrc_mac_get_dstaddr(gnrc_pktsnip_t* pkt, uint8_t* pointer_to_addr[]);


I don't know exactly what it was, but there was a problem with array parameters. Please use uint8_t ** pointer instead

miri64 · 2016-11-29T17:04:50Z

sys/include/net/gnrc/mac/internal.h

+ *
+ * @return                          length of destination address
+ */
+int gnrc_mac_get_dstaddr(gnrc_pktsnip_t* pkt, uint8_t* pointer_to_addr[]);


It seems a bit arbitrary to put a gnrc_netif_hdr specific function here. I would rather put it in that header.

you mean to put this function into sys/include/net/gnrc/netif/hdr?

miri64 · 2016-11-29T17:05:11Z

sys/include/net/gnrc/mac/internal.h

+ *
+ * @return              pointer to data, NULL is not found
+ */
+void* gnrc_mac_pktbuf_find(gnrc_pktsnip_t* pkt, gnrc_nettype_t type);


Anything speaking against using gnrc_pktsnip_search_type() here?

No, just lack of awareness. ;-) will remove this function.

miri64 · 2016-11-29T17:05:27Z

sys/include/net/gnrc/mac/internal.h

+ *
+ * @return              true if the packet is for broadcast, otherwise false
+ */
+static inline bool gnrc_mac_chk_pkt_bcast(gnrc_pktsnip_t* pkt)


It seems a bit arbitrary to put a gnrc_netif_hdr specific function here. I would rather put it in that header.

miri64 · 2016-11-29T17:06:38Z

sys/include/net/gnrc/mac/internal.h

+    assert(addr2);
+    assert(add_len);
+
+    return (memcmp(addr1, addr2, add_len) == 0);


Do you really need this function if it just wraps around memcpy()

I guess not really, just want to feel straightforward when using. So, I remove this function. ;-)

zhuoshuguo · 2016-11-29T17:48:14Z

@miri64 Updated.

zhuoshuguo · 2016-11-29T17:50:44Z

sys/include/net/gnrc/mac/internal.h

+ */
+
+#ifndef GNRC_MAC_INTERNAL_H_
+#define GNRC_MAC_INTERNAL_H_


@miri64 Should I also delete this internal.h (and internal.c and gnrc_mac makefile etc.) which contain nothing now in this PR, and introduce it in PR6074 ?

I think I should~

zhuoshuguo · 2016-11-29T17:51:07Z

sys/include/net/gnrc/netif/hdr.h

+ *
+ * @return              true if the packet is for broadcast, otherwise false
+ */
+static inline bool gnrc_netif_chk_pkt_bcast(gnrc_pktsnip_t* pkt)


How about this ? ;-) @miri64

gnrc_netif_hdr_chk_...

zhuoshuguo · 2016-11-29T17:51:52Z

sys/include/net/gnrc/netif/hdr.h

+ *
+ * @return                          length of destination address
+ */
+int gnrc_netif_get_dstaddr(gnrc_pktsnip_t* pkt, uint8_t** pointer_to_addr);


How about this ? ;-) @miri64

gnrc_netif_hdr_get_...

zhuoshuguo · 2016-11-29T18:10:57Z

sys/include/net/gnrc/netif/hdr.h

+ */
+static inline bool gnrc_netif_chk_pkt_bcast(gnrc_pktsnip_t* pkt)
+{
+    gnrc_netif_hdr_t* netif_hdr = gnrc_mac_pktbuf_find(pkt, GNRC_NETTYPE_NETIF);


my fault, still need some polish here.. will update soon.

zhuoshuguo · 2016-11-29T18:31:51Z

@miri64 Updated again.

zhuoshuguo · 2016-11-29T18:35:44Z

sys/include/net/gnrc/netif/hdr.h

+ *
+ * @return              true if the packet is for broadcast, otherwise false
+ */
+bool gnrc_netif_hdr_chk_pkt_bcast(gnrc_pktsnip_t* pkt);


@miri64 Do you think I should restrict the input to gnrc_netif_hdr_t *, but not for gnrc_pktsnip_t*

Yeah, I guess that would be the better idea. I also would prefer to still distinguish multicast and broadcast here.

miri64 · 2016-11-29T19:03:00Z

sys/include/net/gnrc/netif/hdr.h

+ *
+ * @return                          length of destination address
+ */
+int gnrc_netif_hdr_get_dstaddr(gnrc_pktsnip_t* pkt, uint8_t** pointer_to_addr);


Maybe also add an equivalent function for source address?

@miri64 Yes, good idea, I will do it.;-)

zhuoshuguo · 2016-11-29T19:47:25Z

@miri64 Updated again~

zhuoshuguo · 2016-11-29T19:48:41Z

sys/include/net/gnrc/netif/hdr.h

+ *
+ * @return              true if the packet is for multicast, otherwise false
+ */
+static inline bool gnrc_netif_hdr_chk_pkt_mcast(gnrc_netif_hdr_t *hdr)


@miri64 Added check-multicast function also.

zhuoshuguo · 2016-11-29T19:49:06Z

sys/include/net/gnrc/netif/hdr.h

+ *
+ * @return                          length of destination address
+ */
+int gnrc_netif_hdr_get_srcaddr(gnrc_netif_hdr_t *hdr, uint8_t** pointer_to_addr);


@miri64 Added get-source_address function.

miri64 · 2016-11-29T20:01:52Z

sys/include/net/gnrc/netif/hdr.h

+ *
+ * @return                          length of destination address
+ */
+int gnrc_netif_hdr_get_dstaddr(gnrc_netif_hdr_t *hdr, uint8_t** pointer_to_addr);


Without the search functionality they are pretty much equivalent to http://doc.riot-os.org/group__net__gnrc__netif__hdr.html#gaae99832a0efea4a5a2501a8fc9469f21...

@miri64 I understand, indeed, without the search functionality, it is just a very thin wrap... We just removed a more thick wrapper (the search func.) just now. So, do you think we should come back to the original state, where the input is gnrc_pktsnip_t* and there will be the search functionality (I think this functionality is quite frequently used, at least in MAC)?

Maybe still put that function (get_dstaddress(gnrc_pktsnip_t* pkt)) to gnrc_mac module?

For the address stuff yes. For the flag stuff (bcast, mcast) I think it's best if they just return the flags. Otherwise you need to search for the netif header twice. The flags can then be checked externally (if necessary with a wrapper)

For the address stuff yes.

So, for this, I return to the original state to gnrc_mac_get_dstaddr(gnrc_pktsnip_t* pkt) (with search functionality) and keep this function in gnrc_mac internal?

For the flag stuff (bcast, mcast) I think it's best if they just return the flags. Otherwise you need to search for the netif header twice. The flags can then be checked externally (if necessary with a wrapper)

Fot this, you mean a solution like: uint8_t (flag) gnrc_netif_hdr_get_flags(gnrc_netif_hdr_t *hdr) , right? ;-)

So, for this, I return to the original state to gnrc_mac_get_dstaddr(gnrc_pktsnip_t* pkt) (with search functionality) […]

Yes (but not the name)

and keep this function in gnrc_mac internal?

No. Keep it here.

Fot this, you mean a solution like: uint8_t (flag) gnrc_netif_hdr_get_flags(gnrc_netif_hdr_t *hdr) , right? ;-)

Yes.

@miri64 Got it! will adjust tomorrow~ And, thanks a lot for the huge help tonight ! :-)

miri64 · 2016-12-01T09:20:38Z

sys/include/net/gnrc/netif/hdr.h

+ * @param[out]  pointer_to_addr     pointer to address will be stored here
+ *
+ * @return                          length of destination address
+ * @return                          -ENOENT, if no netif header is presented in @p pkt; or if no


Semicolon doesn't make any sense here language-wise.

miri64 · 2016-12-01T09:20:46Z

sys/include/net/gnrc/netif/hdr.h

+ * @param[out]  pointer_to_addr     pointer to address will be stored here
+ *
+ * @return                          length of source address
+ * @return                          -ENOENT, if no netif header is presented in @p pkt; or if no


miri64 · 2016-12-01T09:22:08Z

sys/net/gnrc/netif/hdr/gnrc_netif_hdr.c

+
+    assert(pkt != NULL);
+    pkt = gnrc_pktsnip_search_type(pkt, GNRC_NETTYPE_NETIF);
+    if(pkt) {


Style (space between if and ().

miri64 · 2016-12-01T09:22:30Z

sys/net/gnrc/netif/hdr/gnrc_netif_hdr.c

+    pkt = gnrc_pktsnip_search_type(pkt, GNRC_NETTYPE_NETIF);
+    if(pkt) {
+        netif_hdr = pkt->data;
+        if(netif_hdr) {


miri64 · 2016-12-01T09:22:45Z

sys/net/gnrc/netif/hdr/gnrc_netif_hdr.c

+
+    assert(pkt != NULL);
+    pkt = gnrc_pktsnip_search_type(pkt, GNRC_NETTYPE_NETIF);
+    if(pkt) {


miri64 · 2016-12-01T09:22:50Z

sys/net/gnrc/netif/hdr/gnrc_netif_hdr.c

+    pkt = gnrc_pktsnip_search_type(pkt, GNRC_NETTYPE_NETIF);
+    if(pkt) {
+        netif_hdr = pkt->data;
+        if(netif_hdr) {


miri64 · 2016-12-01T09:22:55Z

sys/net/gnrc/netif/hdr/gnrc_netif_hdr.c

+    if(pkt) {
+        netif_hdr = pkt->data;
+        if(netif_hdr) {
+            if((res = netif_hdr->dst_l2addr_len) <= 0) {


miri64 · 2016-12-01T09:23:01Z

sys/net/gnrc/netif/hdr/gnrc_netif_hdr.c

+
+    assert(pkt != NULL);
+    pkt = gnrc_pktsnip_search_type(pkt, GNRC_NETTYPE_NETIF);
+    if(pkt) {


miri64 · 2016-12-01T09:23:05Z

sys/net/gnrc/netif/hdr/gnrc_netif_hdr.c

+    pkt = gnrc_pktsnip_search_type(pkt, GNRC_NETTYPE_NETIF);
+    if(pkt) {
+        netif_hdr = pkt->data;
+        if(netif_hdr) {


miri64 · 2016-12-01T09:23:10Z

sys/net/gnrc/netif/hdr/gnrc_netif_hdr.c

+    if(pkt) {
+        netif_hdr = pkt->data;
+        if(netif_hdr) {
+            if((res = netif_hdr.src_l2addr_len) <= 0) {


miri64 · 2016-12-01T09:24:41Z

Apart from minor style and grammar things we can go, as soon as they are fixed.

zhuoshuguo · 2016-12-01T09:27:43Z

@miri64 Adopted all the style comments on space, and turned semicolon to comma.

miri64

One last grammar thing

miri64 · 2016-12-01T10:34:04Z

sys/include/net/gnrc/netif/hdr.h

+ * @param[out]  pointer_to_addr     pointer to address will be stored here
+ *
+ * @return                          length of destination address
+ * @return                          -ENOENT, if no netif header is presented in @p pkt, or if no


No comma, no semicolon. "or" suffices in this case ;-)

Ha~ OK, just to be sure, like: if no netif header is presented in @p pkt or if no ... ?

miri64 · 2016-12-01T10:34:15Z

sys/include/net/gnrc/netif/hdr.h

+ * @param[out]  pointer_to_addr     pointer to address will be stored here
+ *
+ * @return                          length of source address
+ * @return                          -ENOENT, if no netif header is presented in @p pkt, or if no


zhuoshuguo · 2016-12-01T10:43:04Z

Remove comma~

miri64 · 2016-12-01T11:48:02Z

Please squash

zhuoshuguo · 2016-12-01T12:31:47Z

Squashed.

miri64

Murdock found some errors. Please update and squash immediately.

miri64 · 2016-12-02T05:56:21Z

sys/net/gnrc/netif/hdr/gnrc_netif_hdr.c

@@ -38,4 +38,63 @@ gnrc_pktsnip_t *gnrc_netif_hdr_build(uint8_t *src, uint8_t src_len, uint8_t *dst
    return pkt;
 }

+uint8_t gnrc_netif_hdr_get_flag(gnrc_pktsnip_t* pkt)
+{
+    int res;


Variable is not used. Please remove

miri64 · 2016-12-02T05:58:03Z

sys/net/gnrc/netif/hdr/gnrc_netif_hdr.c

+    if (pkt) {
+        netif_hdr = pkt->data;
+        if (netif_hdr) {
+            if ((res = netif_hdr.src_l2addr_len) <= 0) {


Use structure dereference instead (->) of .

zhuoshuguo · 2016-12-02T12:24:08Z

Updated and squahsed again. (Sorry for the error, should have compiled locally to check..)

miri64

ACK and go.

zhuoshuguo mentioned this pull request Nov 8, 2016

gnrc_mac: add mac tx and rx internal types. #6074

Merged

miri64 added GNRC Area: network Area: Networking Type: new feature The issue requests / The PR implemements a new feature for RIOT labels Nov 10, 2016

miri64 added this to the Release 2017.01 milestone Nov 10, 2016

miri64 self-assigned this Nov 10, 2016

zhuoshuguo force-pushed the gnrc_mac_internal_helper branch 5 times, most recently from 497adf2 to dabe766 Compare November 19, 2016 23:42

miri64 requested changes Nov 29, 2016

View reviewed changes

zhuoshuguo commented Nov 29, 2016

View reviewed changes

zhuoshuguo changed the title ~~gnrc_mac: add mac internal helper functions.~~ gnrc_netif: add fetch-destination-address and check-pkt-broadcast functionalities. Nov 29, 2016

zhuoshuguo commented Nov 29, 2016

View reviewed changes

zhuoshuguo force-pushed the gnrc_mac_internal_helper branch from 95b3fb9 to c3e7bc1 Compare November 29, 2016 18:31

zhuoshuguo commented Nov 29, 2016

View reviewed changes

miri64 reviewed Nov 29, 2016

View reviewed changes

zhuoshuguo commented Nov 29, 2016

View reviewed changes

miri64 reviewed Nov 29, 2016

View reviewed changes

miri64 reviewed Dec 1, 2016

View reviewed changes

sys/net/gnrc/netif/hdr/gnrc_netif_hdr.c

assert(pkt != NULL);

pkt = gnrc_pktsnip_search_type(pkt, GNRC_NETTYPE_NETIF);

if(pkt) {

Copy link

Member

miri64 Dec 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dito

miri64 reviewed Dec 1, 2016

View reviewed changes

sys/net/gnrc/netif/hdr/gnrc_netif_hdr.c

assert(pkt != NULL);

pkt = gnrc_pktsnip_search_type(pkt, GNRC_NETTYPE_NETIF);

if(pkt) {

Copy link

Member

miri64 Dec 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dito

miri64 reviewed Dec 1, 2016

View reviewed changes

zhuoshuguo force-pushed the gnrc_mac_internal_helper branch from 96ed42e to af4f03f Compare December 1, 2016 09:27

zhuoshuguo changed the title ~~gnrc_netif: add fetch-destination-address and check-pkt-broadcast functionalities.~~ gnrc_netif: add fetch-address and fetch-netif-flag functionalities. Dec 1, 2016

miri64 requested changes Dec 1, 2016

View reviewed changes

zhuoshuguo force-pushed the gnrc_mac_internal_helper branch from af4f03f to d0a7d1a Compare December 1, 2016 10:42

miri64 added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Dec 1, 2016

zhuoshuguo force-pushed the gnrc_mac_internal_helper branch from d0a7d1a to 670f70c Compare December 1, 2016 12:31

miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable labels Dec 2, 2016

miri64 approved these changes Dec 2, 2016

View reviewed changes

miri64 requested changes Dec 2, 2016

View reviewed changes

gnrc_netif: add fetch-address and fetch-netif-flag functionalities.

d749962

zhuoshuguo force-pushed the gnrc_mac_internal_helper branch from 670f70c to d749962 Compare December 2, 2016 12:23

miri64 approved these changes Dec 2, 2016

View reviewed changes

miri64 merged commit be70241 into RIOT-OS:master Dec 2, 2016

gnrc_netif: add fetch-address and fetch-netif-flag functionalities. #6069

gnrc_netif: add fetch-address and fetch-netif-flag functionalities. #6069

Conversation

zhuoshuguo commented Nov 7, 2016 • edited

zhuoshuguo commented Nov 16, 2016

zhuoshuguo commented Nov 20, 2016

zhuoshuguo commented Nov 22, 2016

zhuoshuguo commented Nov 29, 2016

miri64 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhuoshuguo commented Nov 29, 2016

zhuoshuguo Nov 29, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhuoshuguo commented Nov 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhuoshuguo commented Nov 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhuoshuguo Nov 29, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miri64 commented Dec 1, 2016

zhuoshuguo commented Dec 1, 2016 • edited

miri64 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhuoshuguo commented Dec 1, 2016

miri64 commented Dec 1, 2016

zhuoshuguo commented Dec 1, 2016

miri64 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhuoshuguo commented Dec 2, 2016

miri64 left a comment

Choose a reason for hiding this comment

zhuoshuguo commented Nov 7, 2016 •

edited

zhuoshuguo Nov 29, 2016 •

edited

zhuoshuguo Nov 29, 2016 •

edited

zhuoshuguo commented Dec 1, 2016 •

edited