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

drop the need for OBJ_CAST() when logging netlink objects #421

Merged
merged 4 commits into from
Nov 15, 2023

Conversation

KanjiMonster
Copy link
Contributor

Provide appropriate operator<<s for various rtnl_* structs instead of forcing callers to OBJ_CAST() them, and drop OBJ_CAST() from all users.

Motivation and Context

For some netlink structs we have a operator<<, for others these must be cast to nl_object * via OBJ_CAST(). This makes this error prone, as one might forget to hast (which then causes just he object's address to be printed, or add a OBJ_CAST() when one isn't needed, which may even cause crashes if the cast object isn't a nl_object (e.g. nl_addr).

So handle this casting via appropriate operator<<s, and reduce the possibilities of errors.

How Has This Been Tested?

Built and run tested with debugging enabled.

@KanjiMonster KanjiMonster force-pushed the jogo_no_objcast branch 2 times, most recently from 9c49559 to a6fe147 Compare November 13, 2023 12:43
@KanjiMonster KanjiMonster marked this pull request as ready for review November 13, 2023 12:49
@KanjiMonster KanjiMonster marked this pull request as draft November 13, 2023 15:16
@KanjiMonster
Copy link
Contributor Author

../git/src/netlink/nl_vlan.cc: In member function 'int basebox::nl_vlan::disable_vlan(uint32_t, uint16_t, bool, uint16_t)':
../git/src/netlink/nl_vlan.cc:263:35: warning: the address of 'int link(const char*, const char*)' will never be NULL [-Waddress]
  263 |                << ") of link " << link;
      |

???

int nl_vlan::disable_vlan(uint32_t port_id, uint16_t vid, bool tagged,
                          uint16_t vrf_id) {
  assert(swi);

  // remove vid at ingress
  int rv = remove_ingress_vlan(port_id, vid, tagged, vrf_id);
  if (rv < 0) {
    LOG(ERROR) << __FUNCTION__ << ": failed with rv=" << rv
               << " to remove vid=" << vid << "(tagged=" << tagged
               << ") of link " << link;

hm, yes, there is no variable "link". lol.

@KanjiMonster
Copy link
Contributor Author

This issue already existed before, but was hidden due to the OBJ_CAST().

@KanjiMonster
Copy link
Contributor Author

Pushed fixes for the unearthed issues.

@KanjiMonster KanjiMonster marked this pull request as ready for review November 13, 2023 16:11
@rubensfig
Copy link
Contributor

This MR will need to be rebased after #417, as it created a merge conflict locally for me.

@KanjiMonster
Copy link
Contributor Author

Yepp, whichever you merge first the other one will need to be rebased.

Instead of forcing users to OBJ_CAST() depending on the type, add
helpers for the types to automatically OBJ_CAST().

Signed-off-by: Jonas Gorski <[email protected]>
Now that we have helpers for OBJ_CAST()ing rtnl_* structs as needed, we
can drop the OBJ_CAST()s and directly pass the netlink structs.

Replacement has been done in via

  %s/<< OBJ_CAST(\(.*\))/<< \1/g

with additional manual fixups where needed.

Signed-off-by: Jonas Gorski <[email protected]>
When factoring out the disable vlan code into its functions the error
logs weren't updated from link to port_id, but due to the OBJ_CAST()
this was hidden. After dropping the OBJ_CAST(), g++ started complaining,
so fix the error logs to use port_id.

Fixes the following build warnings:

../git/src/netlink/nl_vlan.cc: In member function 'int basebox::nl_vlan::disable_vlan(uint32_t, uint16_t, bool, uint16_t)':
../git/src/netlink/nl_vlan.cc:263:35: warning: the address of 'int link(const char*, const char*)' will never be NULL [-Waddress]
  263 |                << ") of link " << link;
      |                                   ^~~~
../git/src/netlink/nl_vlan.cc:273:19: warning: the address of 'int link(const char*, const char*)' will never be NULL [-Waddress]
  273 |                << link;
      |                   ^~~~
../git/src/netlink/nl_vlan.cc:288:62: warning: the address of 'int link(const char*, const char*)' will never be NULL [-Waddress]
  288 |                << " to remove vid=" << vid << " of link " << link;
      |

Fixes: 57832c0 ("nl_vlan: factor out enabling/disabling vlan flows/groups")
Signed-off-by: Jonas Gorski <[email protected]>
There is no link variable in this nl_vxlan::create_endpoint(), only
vxlan_link and br_link. This was hidden due to the OBJ_CAST(), but when
the OBJ_CAST() was dropped, g++ started to complain.

Fix this by replacing link with neigh, since neigh is what the text
before the variable says.

Fixes the following warning:

../git/src/netlink/nl_vxlan.cc: In member function 'int basebox::nl_vxlan::create_endpoint(rtnl_link*, rtnl_link*, nl_addr*)':
../git/src/netlink/nl_vxlan.cc:674:45: warning: the address of 'int link(const char*, const char*)' will never be NULL [-Waddress]
  674 |                  << ") to add l2 neigh " << link;
      |                                             ^~~~

Fixes: ec04fe2 ("vxlan: add existing neighbors for new endpoint")
Signed-off-by: Jonas Gorski <[email protected]>
@rubensfig
Copy link
Contributor

Ready for rebase then

@KanjiMonster
Copy link
Contributor Author

Ready for rebase then

Too slow, already rebased. ;P

@rubensfig rubensfig merged commit f12df92 into main Nov 15, 2023
5 checks passed
@rubensfig rubensfig deleted the jogo_no_objcast branch November 15, 2023 12:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants