Re: [PATCH 2/7] netlink, pasta: Split MTU setting functionality out of nl_link_up()

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3846 bytes --]

On Thu, Aug 15, 2024 at 12:54:24AM +0200, Stefano Brivio wrote:
> As we'll use nl_link_up() for more than just bringing up devices, it
> will become awkward to carry empty MTU values around whenever we call
> it.

I prefer clearer single purpose functions too.

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> 
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> ---
>  netlink.c | 35 +++++++++++++++++++++++++----------
>  netlink.h |  3 ++-
>  pasta.c   |  7 +++++--
>  3 files changed, 32 insertions(+), 13 deletions(-)
> 
> diff --git a/netlink.c b/netlink.c
> index e6a315e..e33765e 100644
> --- a/netlink.c
> +++ b/netlink.c
> @@ -942,14 +942,14 @@ int nl_link_set_mac(int s, unsigned int ifi, const void *mac)
>  }
>  
>  /**
> - * nl_link_up() - Bring link up
> + * nl_link_set_mtu() - Set link MTU
>   * @s:		Netlink socket
>   * @ifi:	Interface index
> - * @mtu:	If non-zero, set interface MTU
> + * @mtu:	Interface MTU
>   *
>   * Return: 0 on success, negative error code on failure
>   */
> -int nl_link_up(int s, unsigned int ifi, int mtu)
> +int nl_link_set_mtu(int s, unsigned int ifi, int mtu)
>  {
>  	struct req_t {
>  		struct nlmsghdr nlh;
> @@ -959,17 +959,32 @@ int nl_link_up(int s, unsigned int ifi, int mtu)
>  	} req = {
>  		.ifm.ifi_family	  = AF_UNSPEC,
>  		.ifm.ifi_index	  = ifi,
> -		.ifm.ifi_flags	  = IFF_UP,
> -		.ifm.ifi_change	  = IFF_UP,
>  		.rta.rta_type	  = IFLA_MTU,
>  		.rta.rta_len	  = RTA_LENGTH(sizeof(unsigned int)),
>  		.mtu		  = mtu,
>  	};
> -	ssize_t len = sizeof(req);
>  
> -	if (!mtu)
> -		/* Shorten request to drop MTU attribute */
> -		len = offsetof(struct req_t, rta);
> +	return nl_do(s, &req, RTM_NEWLINK, 0, sizeof(req));
> +}
> +
> +/**
> + * nl_link_up() - Bring link up
> + * @s:		Netlink socket
> + * @ifi:	Interface index
> + *
> + * Return: 0 on success, negative error code on failure
> + */
> +int nl_link_up(int s, unsigned int ifi)
> +{
> +	struct req_t {
> +		struct nlmsghdr nlh;
> +		struct ifinfomsg ifm;
> +	} req = {
> +		.ifm.ifi_family	  = AF_UNSPEC,
> +		.ifm.ifi_index	  = ifi,
> +		.ifm.ifi_flags	  = IFF_UP,
> +		.ifm.ifi_change	  = IFF_UP,
> +	};
>  
> -	return nl_do(s, &req, RTM_NEWLINK, 0, len);
> +	return nl_do(s, &req, RTM_NEWLINK, 0, sizeof(req));
>  }
> diff --git a/netlink.h b/netlink.h
> index 3a1f0de..87d27ae 100644
> --- a/netlink.h
> +++ b/netlink.h
> @@ -23,6 +23,7 @@ int nl_addr_dup(int s_src, unsigned int ifi_src,
>  		int s_dst, unsigned int ifi_dst, sa_family_t af);
>  int nl_link_get_mac(int s, unsigned int ifi, void *mac);
>  int nl_link_set_mac(int s, unsigned int ifi, const void *mac);
> -int nl_link_up(int s, unsigned int ifi, int mtu);
> +int nl_link_set_mtu(int s, unsigned int ifi, int mtu);
> +int nl_link_up(int s, unsigned int ifi);
>  
>  #endif /* NETLINK_H */
> diff --git a/pasta.c b/pasta.c
> index 615ff7b..3a0652e 100644
> --- a/pasta.c
> +++ b/pasta.c
> @@ -288,7 +288,7 @@ void pasta_ns_conf(struct ctx *c)
>  {
>  	int rc = 0;
>  
> -	rc = nl_link_up(nl_sock_ns, 1 /* lo */, 0);
> +	rc = nl_link_up(nl_sock_ns, 1 /* lo */);
>  	if (rc < 0)
>  		die("Couldn't bring up loopback interface in namespace: %s",
>  		    strerror(-rc));
> @@ -303,7 +303,10 @@ void pasta_ns_conf(struct ctx *c)
>  		    strerror(-rc));
>  
>  	if (c->pasta_conf_ns) {
> -		nl_link_up(nl_sock_ns, c->pasta_ifi, c->mtu);
> +		if (c->mtu != -1)
> +			nl_link_set_mtu(nl_sock_ns, c->pasta_ifi, c->mtu);
> +
> +		nl_link_up(nl_sock_ns, c->pasta_ifi);
>  
>  		if (c->ifi4) {
>  			if (c->ip4.no_copy_addrs) {

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]