diff mbox series

[libnetfilter_queue,1/2] pktbuff: add __pktb_setup()

Message ID 20200509091141.10619-1-pablo@netfilter.org
State Under Review
Delegated to: Pablo Neira
Headers show
Series [libnetfilter_queue,1/2] pktbuff: add __pktb_setup() | expand

Commit Message

Pablo Neira Ayuso May 9, 2020, 9:11 a.m. UTC
Add private helper function to set up the pkt_buff object.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 src/extra/pktbuff.c | 54 +++++++++++++++++++++++++++------------------
 1 file changed, 32 insertions(+), 22 deletions(-)

Comments

Duncan Roe May 13, 2020, 6:41 a.m. UTC | #1
On Sat, May 09, 2020 at 11:11:40AM +0200, Pablo Neira Ayuso wrote:
> Add private helper function to set up the pkt_buff object.
>
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
>  src/extra/pktbuff.c | 54 +++++++++++++++++++++++++++------------------
>  1 file changed, 32 insertions(+), 22 deletions(-)
>
> diff --git a/src/extra/pktbuff.c b/src/extra/pktbuff.c
> index 6dd0ca98aff2..118ad898f63b 100644
> --- a/src/extra/pktbuff.c
> +++ b/src/extra/pktbuff.c
> @@ -29,6 +29,34 @@
>   * @{
>   */
>
> +static int __pktb_setup(int family, struct pkt_buff *pktb)

Apart from breakage, this is pktb_setup_family() as I posted in
https://www.spinics.net/lists/netfilter-devel/msg66710.html, with args reversed.

I also added pktb_setup_metadata() to capture 3 other duplicate code lines,
you may not think that worth doing: personal choice I guess.

> +{
> +	struct ethhdr *ethhdr;
> +
> +	switch (family) {
> +	case AF_INET:
> +	case AF_INET6:
> +		pktb->network_header = pktb->data;
> +		break;
> +	case AF_BRIDGE:
> +		ethhdr = (struct ethhdr *)pktb->data;
> +		pktb->mac_header = pktb->data;
> +
> +		switch(ethhdr->h_proto) {
> +		case ETH_P_IP:
> +		case ETH_P_IPV6:
> +			pktb->network_header = pktb->data + ETH_HLEN;
> +			break;
> +		default:
> +			/* This protocol is unsupported. */

Should have moved 'errno = EPROTONOSUPPORT;' here rather than leaving it in
pktb_alloc(). As things stand, pktb_setup() will leave errno unaltered when
this error occurs.

> +			return -1;
> +		}
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
>  /**
>   * pktb_alloc - allocate a new packet buffer
>   * \param family Indicate what family. Currently supported families are
> @@ -52,7 +80,6 @@ EXPORT_SYMBOL
>  struct pkt_buff *pktb_alloc(int family, void *data, size_t len, size_t extra)
>  {
>  	struct pkt_buff *pktb;
> -	struct ethhdr *ethhdr;
>  	void *pkt_data;
>
>  	pktb = calloc(1, sizeof(struct pkt_buff) + len + extra);
> @@ -68,28 +95,11 @@ struct pkt_buff *pktb_alloc(int family, void *data, size_t len, size_t extra)
>
>  	pktb->data = pkt_data;
>
> -	switch(family) {
> -	case AF_INET:
> -	case AF_INET6:
> -		pktb->network_header = pktb->data;
> -		break;
> -	case AF_BRIDGE:
> -		ethhdr = (struct ethhdr *)pktb->data;
> -		pktb->mac_header = pktb->data;
> -
> -		switch(ethhdr->h_proto) {
> -		case ETH_P_IP:
> -		case ETH_P_IPV6:
> -			pktb->network_header = pktb->data + ETH_HLEN;
> -			break;
> -		default:
> -			/* This protocol is unsupported. */
> -			errno = EPROTONOSUPPORT;
> -			free(pktb);
> -			return NULL;
> -		}
> -		break;
> +	if (__pktb_setup(family, pktb) < 0) {
> +		errno = EPROTONOSUPPORT;

As before, above line belongs in __pktb_setup()

> +		free(pktb);

You need 'return NULL;' here, as in the original code.

>  	}
> +
>  	return pktb;
>  }
>
> --
> 2.20.1
>
diff mbox series

Patch

diff --git a/src/extra/pktbuff.c b/src/extra/pktbuff.c
index 6dd0ca98aff2..118ad898f63b 100644
--- a/src/extra/pktbuff.c
+++ b/src/extra/pktbuff.c
@@ -29,6 +29,34 @@ 
  * @{
  */
 
+static int __pktb_setup(int family, struct pkt_buff *pktb)
+{
+	struct ethhdr *ethhdr;
+
+	switch (family) {
+	case AF_INET:
+	case AF_INET6:
+		pktb->network_header = pktb->data;
+		break;
+	case AF_BRIDGE:
+		ethhdr = (struct ethhdr *)pktb->data;
+		pktb->mac_header = pktb->data;
+
+		switch(ethhdr->h_proto) {
+		case ETH_P_IP:
+		case ETH_P_IPV6:
+			pktb->network_header = pktb->data + ETH_HLEN;
+			break;
+		default:
+			/* This protocol is unsupported. */
+			return -1;
+		}
+		break;
+	}
+
+	return 0;
+}
+
 /**
  * pktb_alloc - allocate a new packet buffer
  * \param family Indicate what family. Currently supported families are
@@ -52,7 +80,6 @@  EXPORT_SYMBOL
 struct pkt_buff *pktb_alloc(int family, void *data, size_t len, size_t extra)
 {
 	struct pkt_buff *pktb;
-	struct ethhdr *ethhdr;
 	void *pkt_data;
 
 	pktb = calloc(1, sizeof(struct pkt_buff) + len + extra);
@@ -68,28 +95,11 @@  struct pkt_buff *pktb_alloc(int family, void *data, size_t len, size_t extra)
 
 	pktb->data = pkt_data;
 
-	switch(family) {
-	case AF_INET:
-	case AF_INET6:
-		pktb->network_header = pktb->data;
-		break;
-	case AF_BRIDGE:
-		ethhdr = (struct ethhdr *)pktb->data;
-		pktb->mac_header = pktb->data;
-
-		switch(ethhdr->h_proto) {
-		case ETH_P_IP:
-		case ETH_P_IPV6:
-			pktb->network_header = pktb->data + ETH_HLEN;
-			break;
-		default:
-			/* This protocol is unsupported. */
-			errno = EPROTONOSUPPORT;
-			free(pktb);
-			return NULL;
-		}
-		break;
+	if (__pktb_setup(family, pktb) < 0) {
+		errno = EPROTONOSUPPORT;
+		free(pktb);
 	}
+
 	return pktb;
 }