Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gnrc_ndp_internal: add capability to add external options to NAs #3747

Merged
merged 2 commits into from
Aug 31, 2015
Merged

gnrc_ndp_internal: add capability to add external options to NAs #3747

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a913c01
gnrc_ndp_internal: add capability to add external options to NAs
miri64 Aug 30, 2015
82924c6
gnrc_ndp: adapt for API change in gnrc_ndp_internal
miri64 Aug 31, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions sys/include/net/gnrc/ndp/internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,9 +80,13 @@ void gnrc_ndp_internal_send_nbr_sol(kernel_pid_t iface, ipv6_addr_t *tgt,
* not be NULL.
* @param[in] supply_tl2a Add target link-layer address option to neighbor
* advertisement if link-layer has addresses.
* @param[in] ext_opts External options for the neighbor advertisement. Leave NULL for none.
Copy link
Member

Choose a reason for hiding this comment

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

so this in might need to be an in,out

Copy link
Member Author

Choose a reason for hiding this comment

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

It doesn't change it really, so no. But I'll document, that it will be released in an error case.

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

Copy link
Member

Choose a reason for hiding this comment

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

for the record: will ext_ops be NULL if it gets released due to an error? If not, I would also want you to add a return value to gnrc_ndp_internal_send_nbr_adv

Copy link
Member Author

Choose a reason for hiding this comment

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

No it will not. To be honest: this functionality is just used at one part in the 6LoWPAN-ND (on handling NSs) and it's at the very end of the function. I just throw it in there and don't care for the result (because I don't use it afterwards). If you see a real use-case where we need a return value, I will a return value, otherwise I see it as a waste of resources (RAM, ROM and my time ;-P)

* **Warning:** these are not tested if they are suitable for a
* neighbor advertisement so be sure to check that.
* **Will be released** in an error case.
*/
void gnrc_ndp_internal_send_nbr_adv(kernel_pid_t iface, ipv6_addr_t *tgt,
ipv6_addr_t *dst, bool supply_tl2a);
void gnrc_ndp_internal_send_nbr_adv(kernel_pid_t iface, ipv6_addr_t *tgt, ipv6_addr_t *dst,
bool supply_tl2a, gnrc_pktsnip_t *ext_opts);

/**
* @brief Handles a SL2A option.
Expand Down
4 changes: 2 additions & 2 deletions sys/net/gnrc/network_layer/ndp/gnrc_ndp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,8 +101,8 @@ void gnrc_ndp_nbr_sol_handle(kernel_pid_t iface, gnrc_pktsnip_t *pkt,
sicmpv6_size -= (opt->len * 8);
}

gnrc_ndp_internal_send_nbr_adv(iface, tgt, &ipv6->src,
ipv6_addr_is_multicast(&ipv6->dst));
gnrc_ndp_internal_send_nbr_adv(iface, tgt, &ipv6->src, ipv6_addr_is_multicast(&ipv6->dst),
NULL);

return;
}
Expand Down
10 changes: 5 additions & 5 deletions sys/net/gnrc/network_layer/ndp/internal/gnrc_ndp_internal.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,10 +149,10 @@ void gnrc_ndp_internal_set_state(gnrc_ipv6_nc_t *nc_entry, uint8_t state)
}
}

void gnrc_ndp_internal_send_nbr_adv(kernel_pid_t iface, ipv6_addr_t *tgt,
ipv6_addr_t *dst, bool supply_tl2a)
void gnrc_ndp_internal_send_nbr_adv(kernel_pid_t iface, ipv6_addr_t *tgt, ipv6_addr_t *dst,
bool supply_tl2a, gnrc_pktsnip_t *ext_opts)
{
gnrc_pktsnip_t *hdr, *pkt = NULL;
gnrc_pktsnip_t *hdr, *pkt = ext_opts;
uint8_t adv_flags = 0;

DEBUG("ndp internal: send neighbor advertisement (iface: %" PRIkernel_pid ", tgt: %s, ",
Expand All @@ -179,11 +179,11 @@ void gnrc_ndp_internal_send_nbr_adv(kernel_pid_t iface, ipv6_addr_t *tgt,

if (l2src_len > 0) {
/* add target address link-layer address option */
pkt = gnrc_ndp_opt_tl2a_build(l2src, l2src_len, NULL);
pkt = gnrc_ndp_opt_tl2a_build(l2src, l2src_len, pkt);

if (pkt == NULL) {
DEBUG("ndp internal: error allocating Target Link-layer address option.\n");
gnrc_pktbuf_release(pkt);
gnrc_pktbuf_release(ext_opts);
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't ext_opts rather be released on the caller side (if desired) by indicating an error as a return value? I am just thinking: The caller might want to reuse the pktsnip, if gnrc_ndp_opt_tl2a_build fails.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point...

Copy link
Member Author

Choose a reason for hiding this comment

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

On the other hand: the releases of pkt below release ext_opts, too. So this would be inconsistent.

Copy link
Member

Choose a reason for hiding this comment

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

Hm, then I would opt for a good documentation for now, stating that ext_ops could be modified after the call to gnrc_ndp_internal_send_nbr_adv

return;
}
}
Expand Down