Remove the return value of node_get_prim_orport()

[teor]

Also, we don't clear the address and port when we fail.

This probably doesn't matter right now, but it matters as soon as we try to change node_get_prim_orport().

We don't check it, so let's remove it, and check for the null address instead.

[teor]

Maybe we should just remove the return value as part of the consistency changes in #23975.

[teor]

This is something we can do as part of the address refactor in #24403.

[teor]

#23874 unconditionally clears the address, so all that is left here is to remove the return value.

[teor]

The 0.3.3 freeze deadline has passed, all these children of #24403 belong in 0.3.4

[neel]

In node_get_prim_orport(), if a ORPort exists, the return value comes from RETURN_IPV4_AP(). It seems RETURN_IPV4_AP() is a macro which returns 0 on succes. However, there is a node_get_prim_dirport() which also calls RETURN_IPV4_AP() for a return value. Should I also modify node_get_prim_dirport() (remove the return value and modify functions depending on it to check for a NULL address)?

[teor]

Replying to neel:

In node_get_prim_orport(), if a ORPort exists, the return value comes from RETURN_IPV4_AP(). It seems RETURN_IPV4_AP() is a macro which returns 0 on succes. However, there is a node_get_prim_dirport() which also calls RETURN_IPV4_AP() for a return value. Should I also modify node_get_prim_dirport() (remove the return value and modify functions depending on it to check for a NULL address)?

Yes, you should modify both the ORPort and DirPort functions.

The functions that call them need to check the returned address using "tor_addr_port_is_valid()". Most callers will pass 0 for "for_listening", because they will be connecting to the address, rather than listening on it,

Most of the callers already do a validity check, or they ignore invalid addresses later on.
We should make sure each function handles invalid addresses correctly.

[neel]

Patch to remove the return value of node_get_prim_orport() and node_get_prim_dirport() (Revision 1)

[neel]

I have a patch for this in b23873-p001.diff.

[teor]

Hi, thanks for this patch!

The functions that call them need to check the returned address using "tor_addr_port_is_valid()". Most callers will pass 0 for "for_listening", because they will be connecting to the address, rather than listening on it,

Please revise the patch to use tor_addr_port_is_valid() not tor_addr_is_null().

These checks are around the wrong way, they should say !tor_addr_port_is_valid():

} else if (node->ipv6_preferred || !tor_addr_is_null(&ipv4_addr.addr)) {

  } else if (!tor_addr_is_null(&ipv4_addr.addr)

[neel]

Patch to remove the return value of node_get_prim_orport() and node_get_prim_dirport() (Revision 2)

[neel]

A new patch is available as b23873-p002.diff​.

[teor]

These checks are around the wrong way, they should say !tor_addr_port_is_valid(), because we want to use IPv6 when there is no valid IPv4:

	  } else if (node->ipv6_preferred ||
	             tor_addr_port_is_valid_ap(&ipv4_addr, 0)) {

  } else if (tor_addr_port_is_valid_ap(&ipv4_addr, 0)

[neel]

Patch to remove the return value of node_get_prim_orport() and node_get_prim_dirport() (Revision 3)

[neel]

I have made the requested changes in the file b23873-p003.diff.

[teor]

Thanks!
Someone should review and compile this patch in the next few weeks.

[nickm]

Thanks, neel! This patch looks correct to me. Let's take it early in the 0.3.4.x series.

The only change that I would suggest is that the function documentation should say that callers should check whether the result is valid before using it. Either you can make that change, or I'll do it when I merge; either way is fine with me.

[teor]

This patch looks fine to me.
We can merge it as long as it compiles and passes the unit tests.

[nickm]

wfm; merging.