UML vector network driver: Adjustments for three function implementations
Subsystems (2)
cover UML vector network driver: Adjustments for three function implementations
From: Markus Elfring <[email protected]>
Date: Sun, 11 Mar 2018 16:06:16 +0100
Some update suggestions were taken into account
from static source code analysis.
Markus Elfring (9):
Delete unnecessary code in user_init_raw_fds()
Less checks in user_init_raw_fds() after error detection
Adjust an error message in user_init_socket_fds()
Delete an unnecessary check before kfree() in user_init_socket_fds() Delete two unnecessary checks before freeaddrinfo() in user_init_socket_fds() Less checks in user_init_socket_fds() after error detection Adjust an error message in user_init_tap_fds()
Less checks in user_init_tap_fds() after error detection
Delete an unnecessary variable initialisation in user_init_tap_fds()
arch/um/drivers/vector_user.c | 133 +++++++++++++++++++----------------------- 1 file changed, 60 insertions(+), 73 deletions(-)
Signature · 1 line
2.16.2
PATCH 1/9 um/drivers/vector_user: Delete unnecessary code in user_init_raw_fds()
From: Markus Elfring <[email protected]>
Date: Sun, 11 Mar 2018 11:36:18 +0100
* One condition check could never be reached with a non-null pointer at the end of this function. Thus remove the corresponding statement.
* Delete an initialisation for the local variable "result" which became unnecessary with this refactoring.
--- arch/um/drivers/vector_user.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
arch/um/drivers/vector_user.c +1 -3
@@ -211,7 +211,7 @@ static struct vector_fds *user_init_raw_fds(struct arglist *ifspec)struct sockaddr_ll sock;int err = -ENOMEM ;char *iface; -struct vector_fds *result =NULL ; +struct vector_fds *result;int optval =1 ;
@@ -276,8 +276,6 @@ static struct vector_fds *user_init_raw_fds(struct arglist *ifspec)os_close_file (rxfd);if (txfd >=0 )os_close_file (txfd); -if (result !=NULL ) -kfree (result);return NULL ; }
Signature · 1 line
2.16.2
Anton Ivanov
maintainerRe: [PATCH 1/9] um/drivers/vector_user: Delete unnecessary code in user_init_raw_fds()
Thanks, well noted.
It still does not fix it completely though.
Re-reading the code it will leak a fd if the malloc for result fails. That return result; there should be inside the conditional falling back to cleanup if the alloc fails.
A.
Quoted 36 lines · SF Markus Elfring
From: Markus Elfring <[email protected]>
Date: Sun, 11 Mar 2018 11:36:18 +0100* One condition check could never be reached with a non-null pointer at the end of this function. Thus remove the corresponding statement.
* Delete an initialisation for the local variable "result" which became unnecessary with this refactoring.
Signed-off-by Markus Elfring <[email protected]>--- arch/um/drivers/vector_user.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)arch/um/drivers/vector_user.c +1
-3@@ -211,7 +211,7 @@ static struct vector_fds *user_init_raw_fds(struct arglist *ifspec)struct sockaddr_ll sock;int err = -ENOMEM ;char *iface; -struct vector_fds *result =NULL ; +struct vector_fds *result;int optval =1 ;@@ -276,8 +276,6 @@ static struct vector_fds *user_init_raw_fds(struct arglist *ifspec)os_close_file (rxfd);if (txfd >=0 )os_close_file (txfd); -if (result !=NULL ) -kfree (result);return NULL ; }
Signature · 4 lines
Anton R. Ivanov Cambridge Greys Limited, England and Wales company No 10273661 http://www.cambridgegreys.com/
PATCH 2/9 um/drivers/vector_user: Less checks in user_init_raw_fds() after error detection
In reply to SF Markus ElfringFrom: Markus Elfring <[email protected]>
Date: Sun, 11 Mar 2018 11:40:14 +0100
Up to two checks could be repeated by the user_init_raw_fds() function during error handling even if the relevant properties can be determined for the involved variables before by source code analysis.
* Adjust jump targets so that an extra check can be omitted at the end.
* Delete an initialisation for the local variable "rxfd" which became unnecessary with this refactoring.
--- arch/um/drivers/vector_user.c | 61 ++++++++++++++++++++----------------------- 1 file changed, 28 insertions(+), 33 deletions(-)
arch/um/drivers/vector_user.c +28 -33
@@ -207,54 +207,46 @@ static struct vector_fds *user_init_tap_fds(struct arglist *ifspec)static struct vector_fds *user_init_raw_fds (struct arglist *ifspec) {struct ifreq ifr; -int rxfd =-1 , txfd =-1 ; +int rxfd, txfd =-1 ;struct sockaddr_ll sock; -int err = -ENOMEM ; -char *iface;struct vector_fds *result;int optval =1 ; +int err; +char *iface =uml_vector_fetch_arg (ifspec,TOKEN_IFNAME ); - - iface =uml_vector_fetch_arg (ifspec,TOKEN_IFNAME ); -if (iface =NULL ) - goto cleanup; +if (!iface) { + err = -ENOMEM ; + goto report_failure; + } rxfd =socket (AF_PACKET ,SOCK_RAW ,ETH_P_ALL ); -if (rxfd =-1 ) { - err = -errno; - goto cleanup; - } +if (rxfd =-1 ) + goto set_error_code; + txfd =socket (AF_PACKET ,SOCK_RAW ,0 );/* Turn off RX on this fd */ -if (txfd =-1 ) { - err = -errno; - goto cleanup; - } +if (txfd =-1 ) + goto set_error_code; +memset (&ifr,0 ,sizeof (ifr));strncpy ((char *)&ifr.ifr_name, iface,sizeof (ifr.ifr_name) -1 ); -if (ioctl (rxfd,SIOCGIFINDEX , (void *) &ifr) <0 ) { - err = -errno; - goto cleanup; - } +if (ioctl (rxfd,SIOCGIFINDEX , (void *) &ifr) <0 ) + goto set_error_code; sock.sll_family =AF_PACKET ; sock.sll_protocol =htons (ETH_P_ALL ); sock.sll_ifindex = ifr.ifr_ifindex; -if (bind (rxfd, - (struct sockaddr *) &sock,sizeof (struct sockaddr_ll )) <0 ) { - err = -errno; - goto cleanup; - } +if (bind (rxfd, (struct sockaddr *)&sock,sizeof (struct sockaddr_ll )) + <0 ) + goto set_error_code; sock.sll_family =AF_PACKET ; sock.sll_protocol =htons (ETH_P_IP ); sock.sll_ifindex = ifr.ifr_ifindex; -if (bind (txfd, - (struct sockaddr *) &sock,sizeof (struct sockaddr_ll )) <0 ) { - err = -errno; - goto cleanup; - } +if (bind (txfd, (struct sockaddr *)&sock,sizeof (struct sockaddr_ll )) + <0 ) + goto set_error_code; if (setsockopt(txfd, SOL_PACKET, PACKET_QDISC_BYPASS,
@@ -270,12 +262,15 @@ static struct vector_fds *user_init_raw_fds(struct arglist *ifspec) result->remote_addr_size =0 ; }return result; -cleanup: -printk (UM_KERN_ERR "user_init_raw: init failed, error %d" , err); -if (rxfd >=0 ) -os_close_file (rxfd); + +set_error_code: + err = -errno; +os_close_file (rxfd); +if (txfd >=0 )os_close_file (txfd); +report_failure: +printk (UM_KERN_ERR "%s: init failed: %d" , __func__, err);return NULL ; }
Signature · 1 line
2.16.2
Re: [2/9] um/drivers/vector_user: Less checks in user_init_raw_fds() after error detection
arch/um/drivers/vector_user.c static struct vector_fds *user_init_raw_fds(struct arglist *ifspec) · line 268
Quoted 3 lines
arch/um/drivers/vector_user.c +3
-0+set_error_code: + err = -errno; +os_close_file (rxfd);
I have taken another look at this change idea.
Now I notice that I should have preserved a sanity check there.
if (rxfd >= 0) os_close_file(rxfd);
Regards,
Markus
PATCH 3/9 um/drivers/vector_user: Adjust an error message in user_init_socket_fds()
In reply to SF Markus ElfringFrom: Markus Elfring <[email protected]>
Date: Sun, 11 Mar 2018 13:53:08 +0100
* Adjust an error message at the end of this function.
* Delete the local variable "err" which became unnecessary with this refactoring.
--- arch/um/drivers/vector_user.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
arch/um/drivers/vector_user.c +3 -2
@@ -305,7 +305,6 @@ bool uml_tap_enable_vnet_headers(int fd)static struct vector_fds *user_init_socket_fds (struct arglist *ifspec,int id) { -int err = -ENOMEM ;int fd =-1 , gairet;struct addrinfo srchints;struct addrinfo dsthints;
@@ -419,7 +418,7 @@ static struct vector_fds *user_init_socket_fds(struct arglist *ifspec, int id) cleanup:if (gairesult !=NULL )freeaddrinfo (gairesult); -printk (UM_KERN_ERR "user_init_socket: init failed, error %d" , err); +if (fd >=0 )os_close_file (fd);if (result !=NULL ) {
@@ -427,6 +426,8 @@ static struct vector_fds *user_init_socket_fds(struct arglist *ifspec, int id)kfree (result->remote_addr);kfree (result); } + +printk (UM_KERN_ERR "%s: init failed: %d" , __func__, -ENOMEM );return NULL ; }
Signature · 1 line
2.16.2
PATCH 4/9 um/drivers/vector_user: Delete an unnecessary check before kfree() in user_init_socket_fds()
In reply to SF Markus ElfringFrom: Markus Elfring <[email protected]>
Date: Sun, 11 Mar 2018 14:00:09 +0100
The kfree() function tests whether its argument is NULL and then returns immediately. Thus the test around the call is not needed.
--- arch/um/drivers/vector_user.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
arch/um/drivers/vector_user.c +1 -2
@@ -422,8 +422,7 @@ static struct vector_fds *user_init_socket_fds(struct arglist *ifspec, int id)if (fd >=0 )os_close_file (fd);if (result !=NULL ) { -if (result->remote_addr !=NULL ) -kfree (result->remote_addr); +kfree (result->remote_addr);kfree (result); }
Signature · 1 line
2.16.2
PATCH 5/9 um/drivers/vector_user: Delete two unnecessary checks before freeaddrinfo() in user_init_socket_fds()
In reply to SF Markus ElfringFrom: Markus Elfring <[email protected]>
Date: Sun, 11 Mar 2018 14:20:46 +0100
The implementation returns from this function if a null pointer was detected in the local variable "gairesult". Thus the check before two calls of the function "freeaddrinfo" is not needed.
--- arch/um/drivers/vector_user.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
arch/um/drivers/vector_user.c +2 -5
@@ -384,9 +384,7 @@ static struct vector_fds *user_init_socket_fds(struct arglist *ifspec, int id) goto cleanup; } -if (gairesult !=NULL ) -freeaddrinfo (gairesult); - +freeaddrinfo (gairesult); gairesult =NULL ; gairet =getaddrinfo (dst, dstport, &dsthints, &gairesult);
@@ -416,8 +414,7 @@ static struct vector_fds *user_init_socket_fds(struct arglist *ifspec, int id)freeaddrinfo (gairesult);return result; cleanup: -if (gairesult !=NULL ) -freeaddrinfo (gairesult); +freeaddrinfo (gairesult);if (fd >=0 )os_close_file (fd);
Signature · 1 line
2.16.2
PATCH 6/9 um/drivers/vector_user: Less checks in user_init_socket_fds() after error detection
In reply to SF Markus ElfringFrom: Markus Elfring <[email protected]>
Date: Sun, 11 Mar 2018 14:56:38 +0100
Two checks could be repeated by the user_init_socket_fds() function during error handling even if the relevant properties can be determined for the involved variables before by source code analysis.
* Adjust jump targets.
* Delete two sanity checks and a call of the function "kfree" which became unnecessary with this refactoring.
--- arch/um/drivers/vector_user.c | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-)
arch/um/drivers/vector_user.c +10 -12
@@ -375,13 +375,13 @@ static struct vector_fds *user_init_socket_fds(struct arglist *ifspec, int id)"socket_open : could not open socket, error = %d" , -errno ); - goto cleanup; + goto free_info; }if (bind (fd, (struct sockaddr *) gairesult->ai_addr, gairesult->ai_addrlen)) { printk(UM_KERN_ERR L2TPV3_BIND_FAIL, errno); - goto cleanup; + goto close_file; }freeaddrinfo (gairesult);
@@ -403,7 +403,8 @@ static struct vector_fds *user_init_socket_fds(struct arglist *ifspec, int id) result->remote_addr =uml_kmalloc ( gairesult->ai_addrlen,UM_GFP_KERNEL );if (result->remote_addr =NULL ) - goto cleanup; + goto free_result; + result->remote_addr_size = gairesult->ai_addrlen; memcpy( result->remote_addr,
@@ -413,16 +414,13 @@ static struct vector_fds *user_init_socket_fds(struct arglist *ifspec, int id) }freeaddrinfo (gairesult);return result; -cleanup: -freeaddrinfo (gairesult); - -if (fd >=0 ) -os_close_file (fd); -if (result !=NULL ) { -kfree (result->remote_addr); -kfree (result); - } +free_result: +kfree (result); +close_file: +os_close_file (fd); +free_info: +freeaddrinfo (gairesult);printk (UM_KERN_ERR "%s: init failed: %d" , __func__, -ENOMEM );return NULL ; }
Signature · 1 line
2.16.2
PATCH 7/9 um/drivers/vector_user: Adjust an error message in user_init_tap_fds()
In reply to SF Markus ElfringFrom: Markus Elfring <[email protected]>
Date: Sun, 11 Mar 2018 15:10:05 +0100
Adjust an error message at the end of this function so that its name will be automatically determined as a parameter.
--- arch/um/drivers/vector_user.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
arch/um/drivers/vector_user.c +2 -1
@@ -192,7 +192,6 @@ static struct vector_fds *user_init_tap_fds(struct arglist *ifspec) }return result; tap_cleanup: -printk (UM_KERN_ERR "user_init_tap: init failed, error %d" , err);if (result !=NULL ) {if (result->rx_fd >=0 )os_close_file (result->rx_fd);
@@ -200,6 +199,8 @@ static struct vector_fds *user_init_tap_fds(struct arglist *ifspec)os_close_file (result->tx_fd);kfree (result); } + +printk (UM_KERN_ERR "%s: init failed: %d" , __func__, err);return NULL ; }
Signature · 1 line
2.16.2
PATCH 8/9 um/drivers/vector_user: Less checks in user_init_tap_fds() after error detection
In reply to SF Markus ElfringFrom: Markus Elfring <[email protected]>
Date: Sun, 11 Mar 2018 15:43:31 +0100
Three checks could be repeated by the user_init_tap_fds() function during error handling even if the relevant properties can be determined for the involved variables before by source code analysis.
* Adjust jump targets.
* Delete three sanity checks and an initialisation (for the local variable "result") which became unnecessary with this refactoring.
--- arch/um/drivers/vector_user.c | 31 +++++++++++++++---------------- 1 file changed, 15 insertions(+), 16 deletions(-)
arch/um/drivers/vector_user.c +15 -16
@@ -123,18 +123,18 @@ static struct vector_fds *user_init_tap_fds(struct arglist *ifspec)struct sockaddr_ll sock;int err = -ENOMEM , offload;char *iface; -struct vector_fds *result =NULL ; +struct vector_fds *result; iface =uml_vector_fetch_arg (ifspec,TOKEN_IFNAME );if (iface =NULL ) {printk (UM_KERN_ERR "uml_tap: failed to parse interface spec\n" ); - goto tap_cleanup; + goto report_failure; } result =uml_kmalloc (sizeof (struct vector_fds ),UM_GFP_KERNEL );if (result =NULL ) {printk (UM_KERN_ERR "uml_tap: failed to allocate file descriptors\n" ); - goto tap_cleanup; + goto report_failure; } result->rx_fd =-1 ; result->tx_fd =-1 ;
@@ -146,7 +146,7 @@ static struct vector_fds *user_init_tap_fds(struct arglist *ifspec) fd =open (PATH_NET_TUN ,O_RDWR );if (fd <0 ) {printk (UM_KERN_ERR "uml_tap: failed to open tun device\n" ); - goto tap_cleanup; + goto free_result; } result->tx_fd = fd;memset (&ifr,0 ,sizeof (ifr));
@@ -156,7 +156,7 @@ static struct vector_fds *user_init_tap_fds(struct arglist *ifspec) err =ioctl (fd,TUNSETIFF , (void *) &ifr);if (err !=0 ) {printk (UM_KERN_ERR "uml_tap: failed to select tap interface\n" ); - goto tap_cleanup; + goto close_tx_fd; } offload =TUN_F_CSUM |TUN_F_TSO4 |TUN_F_TSO6 ;
@@ -168,7 +168,7 @@ static struct vector_fds *user_init_tap_fds(struct arglist *ifspec)if (fd =-1 ) {printk (UM_KERN_ERR "uml_tap: failed to create socket: %i\n" , -errno); - goto tap_cleanup; + goto close_tx_fd; } result->rx_fd = fd;memset (&ifr,0 ,sizeof (ifr));
@@ -176,7 +176,7 @@ static struct vector_fds *user_init_tap_fds(struct arglist *ifspec)if (ioctl (fd,SIOCGIFINDEX , (void *) &ifr) <0 ) {printk (UM_KERN_ERR "uml_tap: failed to set interface: %i\n" , -errno); - goto tap_cleanup; + goto close_rx_fd; } sock.sll_family =AF_PACKET ;
@@ -188,18 +188,17 @@ static struct vector_fds *user_init_tap_fds(struct arglist *ifspec)printk (UM_KERN_ERR "user_init_tap: failed to bind raw pair, err %d\n" , -errno); - goto tap_cleanup; + goto close_rx_fd; }return result; -tap_cleanup: -if (result !=NULL ) { -if (result->rx_fd >=0 ) -os_close_file (result->rx_fd); -if (result->tx_fd >=0 ) -os_close_file (result->tx_fd); -kfree (result); - } +close_rx_fd: +os_close_file (result->rx_fd); +close_tx_fd: +os_close_file (result->tx_fd); +free_result: +kfree (result); +report_failure:printk (UM_KERN_ERR "%s: init failed: %d" , __func__, err);return NULL ; }
Signature · 1 line
2.16.2
PATCH 9/9 um/drivers/vector_user: Delete an unnecessary variable initialisation in user_init_tap_fds()
In reply to SF Markus ElfringFrom: Markus Elfring <[email protected]>
Date: Sun, 11 Mar 2018 15:50:29 +0100
The local variable "fd" will eventually be set to an appropriate value a bit later. Thus omit the explicit initialisation at the beginning.
--- arch/um/drivers/vector_user.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
arch/um/drivers/vector_user.c +1 -2
@@ -119,9 +119,8 @@ struct arglist *uml_parse_vector_ifspec(char *arg)static struct vector_fds *user_init_tap_fds (struct arglist *ifspec) {struct ifreq ifr; -int fd =-1 ;struct sockaddr_ll sock; -int err = -ENOMEM , offload; +int err = -ENOMEM , fd, offload;char *iface;struct vector_fds *result;
Signature · 1 line
2.16.2
Quoted 4 lines
Date: Sun, 11 Mar 2018 16:06:16 +0100
Some update suggestions were taken into account
from static source code analysis.
…
Quoted 9 lines
Delete unnecessary code in
user_init_raw_fds()
Less checks inuser_init_raw_fds()after error detection
Adjust an error message inuser_init_socket_fds()
Delete an unnecessary check beforekfree()inuser_init_socket_fds()Delete two unnecessary checks beforefreeaddrinfo()inuser_init_socket_fds()Less checks inuser_init_socket_fds()after error detection Adjust an error message inuser_init_tap_fds()
Less checks inuser_init_tap_fds()after error detection
Delete an unnecessary variable initialisation inuser_init_tap_fds()
…
Would you like to pick any idea up from this patch series?
Regards,
Markus
Yes,
Next version though, I have some tweaks enqueued for the socket initialization. We can combine them with these.
Quoted 23 lines · SF Markus Elfring
Quoted 4 lines
Date: Sun, 11 Mar 2018 16:06:16 +0100
Some update suggestions were taken into account
from static source code analysis.…
Quoted 4 lines
Delete unnecessary code in
user_init_raw_fds()Less checks inuser_init_raw_fds()after error detection Adjust an error message inuser_init_socket_fds()Delete an unnecessary check beforekfree()in
user_init_socket_fds()Quoted 1 line
Delete two unnecessary checks before
freeaddrinfo()in
user_init_socket_fds()Quoted 4 lines
Less checks in
user_init_socket_fds()after error detection Adjust an error message inuser_init_tap_fds()Less checks inuser_init_tap_fds()after error detection Delete an unnecessary variable initialisation in
user_init_tap_fds()
…Would you like to pick any idea up from this patch series?
Regards,
Markus
Signature · 1 line
Sent from my Android device with K-9 Mail. Please excuse my brevity.
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
User-mode-linux-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel