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.

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

route: support for retrieving a link's permanent address. #362

Closed
wants to merge 2 commits into from

Conversation

NailAgliev
Copy link

Not sure about libnl-route-3.sym. Please check.

@@ -1292,6 +1292,7 @@ global:
rtnl_link_bridge_set_vlan_stats_enabled;
rtnl_link_inet6_get_conf;
rtnl_link_info_ops_get;
rtnl_link_get_perm_addr;
Copy link
Owner

Choose a reason for hiding this comment

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

3.8 is already released. That means the section "libnl_3_8" must not be changed anymore.

Add a new libnl_3_9 section at the bottom of the file.

*
* @return Permanent hardware address or NULL if not set.
*/
struct nl_data *rtnl_link_get_perm_addr(struct rtnl_link *link)
Copy link
Owner

Choose a reason for hiding this comment

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

compare to rtnl_link_get_addr. That one returns a struct nl_addr type. I think that should also be used for the perm-addr, or why not?

if (link->ce_mask & LINK_ATTR_PERM_ADDR) {
uint8_t *addr = nl_data_get(link->perm_addr);
nl_dump(p, "hw %x:%x:%x:%x:%x:%x ", addr[0], addr[1], addr[2],
addr[3], addr[4], addr[5]);
Copy link
Owner

Choose a reason for hiding this comment

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

cannot blindly access the addr array. Must make sure that there are 6 elements there (and do something in case there isn't).

@@ -358,6 +359,10 @@ static int link_clone(struct nl_object *_dst, struct nl_object *_src)
if ((err = rtnl_link_sriov_clone(dst, src)) < 0)
return err;

if (src->perm_addr)
if (!(dst->perm_addr = nl_data_clone(src->perm_addr)))
return -NLE_NOMEM;
Copy link
Owner

Choose a reason for hiding this comment

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

please squash the commits. This second commit just fixes a bug in the previous commit.

@@ -73,6 +73,7 @@ struct rtnl_link {
int l_ns_fd;
pid_t l_ns_pid;
struct rtnl_link_vf *l_vf_list;
struct nl_data * perm_addr;
Copy link
Owner

Choose a reason for hiding this comment

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

also needs to be handled in link_compare().

@NailAgliev
Copy link
Author

I'm sorry for keeping the flow-up delayed. I'm hoping to complete this feature soon.

Any updates that should be taken into consideration after the release of 3.9?

@thom311
Copy link
Owner

thom311 commented Mar 4, 2024

I'm sorry for keeping the flow-up delayed. I'm hoping to complete this feature soon.

Don't worry. Contribute at the pace that works best for you. Thank you!

Any updates that should be taken into consideration after the release of 3.9?

No? I think I don't understand the question.

@NailAgliev
Copy link
Author

Sorry for confusing question. I meant to ask if there something a should take into account while implementing requested changes. For example in libnl-route-3.sym section libnl_3_9 already exist due to release of 3.9, so i should name new section 3.10 right?

@thom311
Copy link
Owner

thom311 commented Mar 5, 2024

libnl_3_9 already exist due to release of 3.9, so i should name new section 3.10 right?

Yes, the section to be extended is the one from the next (future) release. At this time, that is libnl_3_10. Note that this section already exists. Make sure to rebase your patch on top of current main.

@thom311
Copy link
Owner

thom311 commented Oct 30, 2024

A similar feature was in the meantime implemented in commit acf572b (#404).

Closing this.

Thank you for your efforts!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants