[ltt-dev] [UST PATCH] Remove all "usterr.h" references in libusctl/ustcomm

[ltt-dev] [UST PATCH] Remove all "usterr.h" references in libusctl/ustcomm

[Nils Carlson]

This patch removes all references to usterr.h and functions in
usterr.h from libustctl and ustcomm. This is done for three reasons:

1. DBG, ERR, PERROR etc call in ust_safe_snprintf, I don't know
   why, and I can't figure out any good reasons. If somebody
   can enlighten me I would be very grateful. The only thing
   I can figure out is that these are some sort of "signal safe"
   printf.

2. All this stuff is quite stable and working and libraries really
   shouldn't be printing stuff.

3. This allows me to compile lttng-tools without linking libustctl
   to the ust_snprintf family of functions. This makes me happy.

Mathieu, enlighten me as best you can... :-)

Signed-off-by: Nils Carlson <nils.carlson at ericsson.com>
---
 libustcomm/ustcomm.c  |   52 +++++-------------------------------------------
 libustctl/libustctl.c |   13 ++---------
 2 files changed, 9 insertions(+), 56 deletions(-)

diff --git a/libustcomm/ustcomm.c b/libustcomm/ustcomm.c
index dcf8cd8..2a5acc7 100644
--- a/libustcomm/ustcomm.c
+++ b/libustcomm/ustcomm.c
@@ -29,13 +29,14 @@
 #include <sys/epoll.h>
 #include <sys/stat.h>
 
+#include <stdarg.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
 #include <execinfo.h>
 
 #include "ustcomm.h"
-#include "usterr.h"
+#include <ust/core.h>
 #include "share.h"
 
 static int mkdir_p(const char *path, mode_t mode)
@@ -101,7 +102,6 @@ static struct sockaddr_un * create_sock_addr(const char *name,
 
 	addr = malloc(alloc_size);
 	if (addr < 0) {
-		ERR("allocating addr failed");
 		return NULL;
 	}
 
@@ -149,9 +149,7 @@ void ustcomm_del_sock(struct ustcomm_sock *sock, int keep_in_epoll)
 {
 	cds_list_del(&sock->list);
 	if (!keep_in_epoll) {
-		if (epoll_ctl(sock->epoll_fd, EPOLL_CTL_DEL, sock->fd, NULL) == -1) {
-			PERROR("epoll_ctl: failed to delete socket");
-		}
+		epoll_ctl(sock->epoll_fd, EPOLL_CTL_DEL, sock->fd, NULL);
 	}
 	close(sock->fd);
 	free(sock);
@@ -168,13 +166,11 @@ struct ustcomm_sock * ustcomm_init_named_socket(const char *name,
 
 	fd = socket(PF_UNIX, SOCK_STREAM, 0);
 	if(fd == -1) {
-		PERROR("socket");
 		return NULL;
 	}
 
 	addr = create_sock_addr(name, &sock_addr_size);
 	if (addr == NULL) {
-		ERR("allocating addr, UST thread bailing");
 		goto close_sock;
 	}
 
@@ -182,29 +178,24 @@ struct ustcomm_sock * ustcomm_init_named_socket(const char *name,
 	if(result == 0) {
 		/* file exists */
 		result = unlink(name);
-		if(result == -1) {
-			PERROR("unlink of socket file");
+		if (result == -1) {
 			goto free_addr;
 		}
-		DBG("socket already exists; overwriting");
 	}
 
 	result = bind(fd, (struct sockaddr *)addr, sock_addr_size);
 	if(result == -1) {
-		PERROR("bind");
 		goto free_addr;
 	}
 
 	result = listen(fd, 1);
 	if(result == -1) {
-		PERROR("listen");
 		goto free_addr;
 	}
 
 	sock = ustcomm_init_sock(fd, epoll_fd,
 				 NULL);
 	if (!sock) {
-		ERR("failed to create ustcomm_sock");
 		goto free_addr;
 	}
 
@@ -236,39 +227,31 @@ void ustcomm_del_named_sock(struct ustcomm_sock *sock,
 		/* Get the socket name */
 		alloc_size = sizeof(dummy);
 		if (getsockname(fd, &dummy, (socklen_t *)&alloc_size) < 0) {
-			PERROR("getsockname failed");
 			goto del_sock;
 		}
 
 		sockaddr = zmalloc(alloc_size);
 		if (!sockaddr) {
-			ERR("failed to allocate sockaddr");
 			goto del_sock;
 		}
 
 		if (getsockname(fd, sockaddr, (socklen_t *)&alloc_size) < 0) {
-			PERROR("getsockname failed");
 			goto free_sockaddr;
 		}
 
 		/* Destroy socket */
 		result = stat(sockaddr->sun_path, &st);
 		if(result < 0) {
-			PERROR("stat (%s)", sockaddr->sun_path);
 			goto free_sockaddr;
 		}
 
 		/* Paranoid check before deleting. */
 		result = S_ISSOCK(st.st_mode);
 		if(!result) {
-			ERR("The socket we are about to delete is not a socket.");
 			goto free_sockaddr;
 		}
 
-		result = unlink(sockaddr->sun_path);
-		if(result < 0) {
-			PERROR("unlink");
-		}
+		unlink(sockaddr->sun_path);
 	}
 
 free_sockaddr:
@@ -297,7 +280,6 @@ int ustcomm_recv_alloc(int sock,
 		} else if (errno == EINTR) {
 			return -1;
 		} else if (result < 0) {
-			PERROR("recv");
 			return -1;
 		}
 		return 0;
@@ -326,7 +308,6 @@ int ustcomm_recv_alloc(int sock,
 	result = recvmsg(sock, &msg, MSG_WAITALL);
 	if (result < 0) {
 		free(*data);
-		PERROR("recvmsg failed");
 	}
 
 	return result;
@@ -355,7 +336,6 @@ int ustcomm_recv_fd(int sock,
 		} else if (errno == EINTR) {
 			return -1;
 		} else if (result < 0) {
-			PERROR("recv");
 			return -1;
 		}
 		return 0;
@@ -372,7 +352,6 @@ int ustcomm_recv_fd(int sock,
 	if (peek_header.size && data) {
 		if (peek_header.size < 0 ||
 		    peek_header.size > USTCOMM_DATA_SIZE) {
-			ERR("big peek header! %ld", peek_header.size);
 			return 0;
 		}
 
@@ -389,9 +368,6 @@ int ustcomm_recv_fd(int sock,
 
 	result = recvmsg(sock, &msg, MSG_WAITALL);
 	if (result <= 0) {
-		if (result < 0) {
-			PERROR("recvmsg failed");
-		}
 		return result;
 	}
 
@@ -407,9 +383,6 @@ int ustcomm_recv_fd(int sock,
 			}
 			cmsg = CMSG_NXTHDR(&msg, cmsg);
 		}
-		if (!result) {
-			ERR("Failed to receive file descriptor\n");
-		}
 	}
 
 	return 1;
@@ -430,7 +403,6 @@ int ustcomm_send_fd(int sock,
 {
 	struct iovec iov[2];
 	struct msghdr msg;
-	int result;
 	struct cmsghdr *cmsg;
 	char buf[CMSG_SPACE(sizeof(int))];
 
@@ -461,11 +433,7 @@ int ustcomm_send_fd(int sock,
 		msg.msg_controllen = cmsg->cmsg_len;
 	}
 
-	result = sendmsg(sock, &msg, MSG_NOSIGNAL);
-	if (result < 0 && errno != EPIPE) {
-		PERROR("sendmsg failed");
-	}
-	return result;
+	return sendmsg(sock, &msg, MSG_NOSIGNAL);
 }
 
 int ustcomm_send(int sock,
@@ -504,19 +472,16 @@ int ustcomm_connect_path(const char *name, int *connection_fd)
 
 	fd = socket(PF_UNIX, SOCK_STREAM, 0);
 	if(fd == -1) {
-		PERROR("socket");
 		return -1;
 	}
 
 	addr = create_sock_addr(name, &sock_addr_size);
 	if (addr == NULL) {
-		ERR("allocating addr failed");
 		goto close_sock;
 	}
 
 	result = connect(fd, (struct sockaddr *)addr, sock_addr_size);
 	if(result == -1) {
-		PERROR("connect (path=%s)", name);
 		goto free_sock_addr;
 	}
 
@@ -543,7 +508,6 @@ char *ustcomm_user_sock_dir(void)
 	result = asprintf(&sock_dir, "%s%s", USER_SOCK_DIR,
 			  cuserid(NULL));
 	if (result < 0) {
-		ERR("string overflow allocating directory name");
 		return NULL;
 	}
 
@@ -569,14 +533,12 @@ static int connect_app_non_root(pid_t pid, int *app_fd)
 
 	result = asprintf(&sock_name, "%s/%d", dir_name, pid);
 	if (result < 0) {
-		ERR("failed to allocate socket name");
 		retval = -1;
 		goto free_dir_name;
 	}
 
 	result = ustcomm_connect_path(sock_name, app_fd);
 	if (result < 0) {
-		ERR("failed to connect to app");
 		retval = -1;
 		goto free_sock_name;
 	}
@@ -657,14 +619,12 @@ int ensure_dir_exists(const char *dir, mode_t mode)
 
 		result = mkdir_p(dir, mode);
 		if(result != 0) {
-			ERR("executing in recursive creation of directory %s", dir);
 			return -1;
 		}
 	} else {
 		if (st.st_mode != mode) {
 			result = chmod(dir, mode);
 			if (result < 0) {
-				ERR("couldn't set directory mode on %s", dir);
 				return -1;
 			}
 		}
diff --git a/libustctl/libustctl.c b/libustctl/libustctl.c
index 1911434..b5d6a02 100644
--- a/libustctl/libustctl.c
+++ b/libustctl/libustctl.c
@@ -17,6 +17,7 @@
  */
 
 #define _GNU_SOURCE
+#include <errno.h>
 #include <stdio.h>
 #include <unistd.h>
 #include <getopt.h>
@@ -26,8 +27,8 @@
 #include <dirent.h>
 
 #include "ustcomm.h"
-#include "ust/ustctl.h"
-#include "usterr.h"
+#include <ust/ustctl.h>
+#include <ust/core.h>
 
 static int do_cmd(int sock,
 		  const struct ustcomm_header *req_header,
@@ -57,7 +58,6 @@ static int do_cmd(int sock,
 			}
 		}
 	} else {
-		ERR("ustcomm req failed");
 		if (result == 0) {
 			saved_errno = ENOTCONN;
 		} else {
@@ -80,7 +80,6 @@ int ustctl_connect_pid(pid_t pid)
 	int sock;
 
 	if (ustcomm_connect_app(pid, &sock)) {
-		ERR("could not connect to PID %u", (unsigned int) pid);
 		errno = ENOTCONN;
 		return -1;
 	}
@@ -580,20 +579,17 @@ int ustctl_get_cmsf(int sock, struct marker_status **cmsf)
 
 	result = ustcomm_send(sock, &req_header, NULL);
 	if (result <= 0) {
-		PERROR("error while requesting markers list");
 		return -1;
 	}
 
 	result = ustcomm_recv_alloc(sock, &res_header, &big_str);
 	if (result <= 0) {
-		ERR("error while receiving markers list");
 		return -1;
 	}
 
 	tmp_cmsf = (struct marker_status *) zmalloc(sizeof(struct marker_status) *
 						    (ustctl_count_nl(big_str) + 1));
 	if (tmp_cmsf == NULL) {
-		ERR("Failed to allocate CMSF array");
 		return -1;
 	}
 
@@ -672,13 +668,11 @@ int ustctl_get_tes(int sock, struct trace_event_status **tes)
 
 	result = ustcomm_send(sock, &req_header, NULL);
 	if (result != 1) {
-		ERR("error while requesting trace_event list");
 		return -1;
 	}
 
 	result = ustcomm_recv_alloc(sock, &res_header, &big_str);
 	if (result != 1) {
-		ERR("error while receiving markers list");
 		return -1;
 	}
 
@@ -686,7 +680,6 @@ int ustctl_get_tes(int sock, struct trace_event_status **tes)
 		zmalloc(sizeof(struct trace_event_status) *
 			(ustctl_count_nl(big_str) + 1));
 	if (tmp_tes == NULL) {
-		ERR("Failed to allocate TES array");
 		return -1;
 	}
 
-- 
1.7.1

[ltt-dev] [UST PATCH] Remove all "usterr.h" references in libusctl/ustcomm

[Mathieu Desnoyers]

* Nils Carlson (nils.carlson at ericsson.com) wrote:
> This patch removes all references to usterr.h and functions in
> usterr.h from libustctl and ustcomm. This is done for three reasons:
> 
> 1. DBG, ERR, PERROR etc call in ust_safe_snprintf, I don't know
>    why, and I can't figure out any good reasons. If somebody
>    can enlighten me I would be very grateful. The only thing
>    I can figure out is that these are some sort of "signal safe"
>    printf.

Yep, that's right.

> 
> 2. All this stuff is quite stable and working and libraries really
>    shouldn't be printing stuff.

What I disagree about the patch below is that you are removing debug
error printouts without taking care of some of the error values in many
cases (just ignoring them).

So we start from a dubtious behavior (debug error messages was not the
proper way to handle error values; they should rather have been treated
and pushed to the callers), to just ignoring the errors, which is kind
of bad, you know... ;)

So I would expect that all the error values you remove debug messages
from should be dealt with rather than ignored.

> 
> 3. This allows me to compile lttng-tools without linking libustctl
>    to the ust_snprintf family of functions. This makes me happy.

Well, you could just create a set of debug macros that use the standard
printf, and decide at compile-time if the log output is built or not
with these pretty simple macros. This change would make more sense to me
than removing the debugging code we have there.

More comments below,

> 
> Mathieu, enlighten me as best you can... :-)
> 
> Signed-off-by: Nils Carlson <nils.carlson at ericsson.com>
> ---
>  libustcomm/ustcomm.c  |   52 +++++-------------------------------------------
>  libustctl/libustctl.c |   13 ++---------
>  2 files changed, 9 insertions(+), 56 deletions(-)
> 
> diff --git a/libustcomm/ustcomm.c b/libustcomm/ustcomm.c
> index dcf8cd8..2a5acc7 100644
> --- a/libustcomm/ustcomm.c
> +++ b/libustcomm/ustcomm.c
> @@ -29,13 +29,14 @@
>  #include <sys/epoll.h>
>  #include <sys/stat.h>
>  
> +#include <stdarg.h>
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <string.h>
>  #include <execinfo.h>
>  
>  #include "ustcomm.h"
> -#include "usterr.h"
> +#include <ust/core.h>
>  #include "share.h"
>  
>  static int mkdir_p(const char *path, mode_t mode)
> @@ -101,7 +102,6 @@ static struct sockaddr_un * create_sock_addr(const char *name,
>  
>  	addr = malloc(alloc_size);
>  	if (addr < 0) {

Hrm, the original test is bogus there ? malloc returns NULL if alloc
fails, not a negative value... ?

> -		ERR("allocating addr failed");
>  		return NULL;
>  	}
>  
> @@ -149,9 +149,7 @@ void ustcomm_del_sock(struct ustcomm_sock *sock, int keep_in_epoll)
>  {
>  	cds_list_del(&sock->list);
>  	if (!keep_in_epoll) {
> -		if (epoll_ctl(sock->epoll_fd, EPOLL_CTL_DEL, sock->fd, NULL) == -1) {
> -			PERROR("epoll_ctl: failed to delete socket");
> -		}
> +		epoll_ctl(sock->epoll_fd, EPOLL_CTL_DEL, sock->fd, NULL);

I think we should keep the error value test here ?

>  	}
>  	close(sock->fd);
>  	free(sock);
> @@ -168,13 +166,11 @@ struct ustcomm_sock * ustcomm_init_named_socket(const char *name,
>  
>  	fd = socket(PF_UNIX, SOCK_STREAM, 0);
>  	if(fd == -1) {
> -		PERROR("socket");
>  		return NULL;
>  	}
>  
>  	addr = create_sock_addr(name, &sock_addr_size);
>  	if (addr == NULL) {
> -		ERR("allocating addr, UST thread bailing");
>  		goto close_sock;
>  	}
>  
> @@ -182,29 +178,24 @@ struct ustcomm_sock * ustcomm_init_named_socket(const char *name,
>  	if(result == 0) {
>  		/* file exists */
>  		result = unlink(name);
> -		if(result == -1) {
> -			PERROR("unlink of socket file");
> +		if (result == -1) {
>  			goto free_addr;
>  		}
> -		DBG("socket already exists; overwriting");
>  	}
>  
>  	result = bind(fd, (struct sockaddr *)addr, sock_addr_size);
>  	if(result == -1) {
> -		PERROR("bind");
>  		goto free_addr;
>  	}
>  
>  	result = listen(fd, 1);
>  	if(result == -1) {
> -		PERROR("listen");
>  		goto free_addr;
>  	}
>  
>  	sock = ustcomm_init_sock(fd, epoll_fd,
>  				 NULL);
>  	if (!sock) {
> -		ERR("failed to create ustcomm_sock");
>  		goto free_addr;
>  	}
>  
> @@ -236,39 +227,31 @@ void ustcomm_del_named_sock(struct ustcomm_sock *sock,
>  		/* Get the socket name */
>  		alloc_size = sizeof(dummy);
>  		if (getsockname(fd, &dummy, (socklen_t *)&alloc_size) < 0) {
> -			PERROR("getsockname failed");
>  			goto del_sock;
>  		}
>  
>  		sockaddr = zmalloc(alloc_size);
>  		if (!sockaddr) {
> -			ERR("failed to allocate sockaddr");
>  			goto del_sock;
>  		}
>  
>  		if (getsockname(fd, sockaddr, (socklen_t *)&alloc_size) < 0) {
> -			PERROR("getsockname failed");
>  			goto free_sockaddr;
>  		}
>  
>  		/* Destroy socket */
>  		result = stat(sockaddr->sun_path, &st);
>  		if(result < 0) {
> -			PERROR("stat (%s)", sockaddr->sun_path);
>  			goto free_sockaddr;
>  		}
>  
>  		/* Paranoid check before deleting. */
>  		result = S_ISSOCK(st.st_mode);
>  		if(!result) {
> -			ERR("The socket we are about to delete is not a socket.");
>  			goto free_sockaddr;
>  		}
>  
> -		result = unlink(sockaddr->sun_path);
> -		if(result < 0) {
> -			PERROR("unlink");
> -		}
> +		unlink(sockaddr->sun_path);

Should we do something with the error here ? Why is it not treated (if
there is a reason, we should add a comment)

Thanks,

Mathieu

>  	}
>  
>  free_sockaddr:
> @@ -297,7 +280,6 @@ int ustcomm_recv_alloc(int sock,
>  		} else if (errno == EINTR) {
>  			return -1;
>  		} else if (result < 0) {
> -			PERROR("recv");
>  			return -1;
>  		}
>  		return 0;
> @@ -326,7 +308,6 @@ int ustcomm_recv_alloc(int sock,
>  	result = recvmsg(sock, &msg, MSG_WAITALL);
>  	if (result < 0) {
>  		free(*data);
> -		PERROR("recvmsg failed");
>  	}
>  
>  	return result;
> @@ -355,7 +336,6 @@ int ustcomm_recv_fd(int sock,
>  		} else if (errno == EINTR) {
>  			return -1;
>  		} else if (result < 0) {
> -			PERROR("recv");
>  			return -1;
>  		}
>  		return 0;
> @@ -372,7 +352,6 @@ int ustcomm_recv_fd(int sock,
>  	if (peek_header.size && data) {
>  		if (peek_header.size < 0 ||
>  		    peek_header.size > USTCOMM_DATA_SIZE) {
> -			ERR("big peek header! %ld", peek_header.size);
>  			return 0;
>  		}
>  
> @@ -389,9 +368,6 @@ int ustcomm_recv_fd(int sock,
>  
>  	result = recvmsg(sock, &msg, MSG_WAITALL);
>  	if (result <= 0) {
> -		if (result < 0) {
> -			PERROR("recvmsg failed");
> -		}
>  		return result;
>  	}
>  
> @@ -407,9 +383,6 @@ int ustcomm_recv_fd(int sock,
>  			}
>  			cmsg = CMSG_NXTHDR(&msg, cmsg);
>  		}
> -		if (!result) {
> -			ERR("Failed to receive file descriptor\n");
> -		}
>  	}
>  
>  	return 1;
> @@ -430,7 +403,6 @@ int ustcomm_send_fd(int sock,
>  {
>  	struct iovec iov[2];
>  	struct msghdr msg;
> -	int result;
>  	struct cmsghdr *cmsg;
>  	char buf[CMSG_SPACE(sizeof(int))];
>  
> @@ -461,11 +433,7 @@ int ustcomm_send_fd(int sock,
>  		msg.msg_controllen = cmsg->cmsg_len;
>  	}
>  
> -	result = sendmsg(sock, &msg, MSG_NOSIGNAL);
> -	if (result < 0 && errno != EPIPE) {
> -		PERROR("sendmsg failed");
> -	}
> -	return result;
> +	return sendmsg(sock, &msg, MSG_NOSIGNAL);
>  }
>  
>  int ustcomm_send(int sock,
> @@ -504,19 +472,16 @@ int ustcomm_connect_path(const char *name, int *connection_fd)
>  
>  	fd = socket(PF_UNIX, SOCK_STREAM, 0);
>  	if(fd == -1) {
> -		PERROR("socket");
>  		return -1;
>  	}
>  
>  	addr = create_sock_addr(name, &sock_addr_size);
>  	if (addr == NULL) {
> -		ERR("allocating addr failed");
>  		goto close_sock;
>  	}
>  
>  	result = connect(fd, (struct sockaddr *)addr, sock_addr_size);
>  	if(result == -1) {
> -		PERROR("connect (path=%s)", name);
>  		goto free_sock_addr;
>  	}
>  
> @@ -543,7 +508,6 @@ char *ustcomm_user_sock_dir(void)
>  	result = asprintf(&sock_dir, "%s%s", USER_SOCK_DIR,
>  			  cuserid(NULL));
>  	if (result < 0) {
> -		ERR("string overflow allocating directory name");
>  		return NULL;
>  	}
>  
> @@ -569,14 +533,12 @@ static int connect_app_non_root(pid_t pid, int *app_fd)
>  
>  	result = asprintf(&sock_name, "%s/%d", dir_name, pid);
>  	if (result < 0) {
> -		ERR("failed to allocate socket name");
>  		retval = -1;
>  		goto free_dir_name;
>  	}
>  
>  	result = ustcomm_connect_path(sock_name, app_fd);
>  	if (result < 0) {
> -		ERR("failed to connect to app");
>  		retval = -1;
>  		goto free_sock_name;
>  	}
> @@ -657,14 +619,12 @@ int ensure_dir_exists(const char *dir, mode_t mode)
>  
>  		result = mkdir_p(dir, mode);
>  		if(result != 0) {
> -			ERR("executing in recursive creation of directory %s", dir);
>  			return -1;
>  		}
>  	} else {
>  		if (st.st_mode != mode) {
>  			result = chmod(dir, mode);
>  			if (result < 0) {
> -				ERR("couldn't set directory mode on %s", dir);
>  				return -1;
>  			}
>  		}
> diff --git a/libustctl/libustctl.c b/libustctl/libustctl.c
> index 1911434..b5d6a02 100644
> --- a/libustctl/libustctl.c
> +++ b/libustctl/libustctl.c
> @@ -17,6 +17,7 @@
>   */
>  
>  #define _GNU_SOURCE
> +#include <errno.h>
>  #include <stdio.h>
>  #include <unistd.h>
>  #include <getopt.h>
> @@ -26,8 +27,8 @@
>  #include <dirent.h>
>  
>  #include "ustcomm.h"
> -#include "ust/ustctl.h"
> -#include "usterr.h"
> +#include <ust/ustctl.h>
> +#include <ust/core.h>
>  
>  static int do_cmd(int sock,
>  		  const struct ustcomm_header *req_header,
> @@ -57,7 +58,6 @@ static int do_cmd(int sock,
>  			}
>  		}
>  	} else {
> -		ERR("ustcomm req failed");
>  		if (result == 0) {
>  			saved_errno = ENOTCONN;
>  		} else {
> @@ -80,7 +80,6 @@ int ustctl_connect_pid(pid_t pid)
>  	int sock;
>  
>  	if (ustcomm_connect_app(pid, &sock)) {
> -		ERR("could not connect to PID %u", (unsigned int) pid);
>  		errno = ENOTCONN;
>  		return -1;
>  	}
> @@ -580,20 +579,17 @@ int ustctl_get_cmsf(int sock, struct marker_status **cmsf)
>  
>  	result = ustcomm_send(sock, &req_header, NULL);
>  	if (result <= 0) {
> -		PERROR("error while requesting markers list");
>  		return -1;
>  	}
>  
>  	result = ustcomm_recv_alloc(sock, &res_header, &big_str);
>  	if (result <= 0) {
> -		ERR("error while receiving markers list");
>  		return -1;
>  	}
>  
>  	tmp_cmsf = (struct marker_status *) zmalloc(sizeof(struct marker_status) *
>  						    (ustctl_count_nl(big_str) + 1));
>  	if (tmp_cmsf == NULL) {
> -		ERR("Failed to allocate CMSF array");
>  		return -1;
>  	}
>  
> @@ -672,13 +668,11 @@ int ustctl_get_tes(int sock, struct trace_event_status **tes)
>  
>  	result = ustcomm_send(sock, &req_header, NULL);
>  	if (result != 1) {
> -		ERR("error while requesting trace_event list");
>  		return -1;
>  	}
>  
>  	result = ustcomm_recv_alloc(sock, &res_header, &big_str);
>  	if (result != 1) {
> -		ERR("error while receiving markers list");
>  		return -1;
>  	}
>  
> @@ -686,7 +680,6 @@ int ustctl_get_tes(int sock, struct trace_event_status **tes)
>  		zmalloc(sizeof(struct trace_event_status) *
>  			(ustctl_count_nl(big_str) + 1));
>  	if (tmp_tes == NULL) {
> -		ERR("Failed to allocate TES array");
>  		return -1;
>  	}
>  
> -- 
> 1.7.1
> 
> 
> _______________________________________________
> ltt-dev mailing list
> ltt-dev at lists.casi.polymtl.ca
> http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev
> 

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

[ltt-dev] [UST PATCH] Remove all "usterr.h" references in libusctl/ustcomm

[Nils Carlson]

On Apr 1, 2011, at 5:14 PM, Mathieu Desnoyers wrote:

> * Nils Carlson (nils.carlson at ericsson.com) wrote:
>> This patch removes all references to usterr.h and functions in
>> usterr.h from libustctl and ustcomm. This is done for three reasons:
>>
>> 1. DBG, ERR, PERROR etc call in ust_safe_snprintf, I don't know
>>   why, and I can't figure out any good reasons. If somebody
>>   can enlighten me I would be very grateful. The only thing
>>   I can figure out is that these are some sort of "signal safe"
>>   printf.
>
> Yep, that's right.
>
>>
>> 2. All this stuff is quite stable and working and libraries really
>>   shouldn't be printing stuff.
>
> What I disagree about the patch below is that you are removing debug
> error printouts without taking care of some of the error values in  
> many
> cases (just ignoring them).
>
> So we start from a dubtious behavior (debug error messages was not the
> proper way to handle error values; they should rather have been  
> treated
> and pushed to the callers), to just ignoring the errors, which is kind
> of bad, you know... ;)
>
> So I would expect that all the error values you remove debug messages
> from should be dealt with rather than ignored.

Hm. errno's are still there... As long as errnos aren't overwritten  
they can still be used. Can't find anywhere where we are making calls  
that should overwrite errnos. We could try to introduce consistency in  
returning negative errno's though. That would be a separate patch.
>
>>
>> 3. This allows me to compile lttng-tools without linking libustctl
>>   to the ust_snprintf family of functions. This makes me happy.
>
> Well, you could just create a set of debug macros that use the  
> standard
> printf, and decide at compile-time if the log output is built or not
> with these pretty simple macros. This change would make more sense  
> to me
> than removing the debugging code we have there.
>
Well, could do that... but as noted, this is pretty stable stuff and  
we can add debug code as needed I think.

> More comments below,
>
>>
>> Mathieu, enlighten me as best you can... :-)
>>
>> Signed-off-by: Nils Carlson <nils.carlson at ericsson.com>
>> ---
>> libustcomm/ustcomm.c  |   52 ++++ 
>> +-------------------------------------------
>> libustctl/libustctl.c |   13 ++---------
>> 2 files changed, 9 insertions(+), 56 deletions(-)
>>
>> diff --git a/libustcomm/ustcomm.c b/libustcomm/ustcomm.c
>> index dcf8cd8..2a5acc7 100644
>> --- a/libustcomm/ustcomm.c
>> +++ b/libustcomm/ustcomm.c
>> @@ -29,13 +29,14 @@
>> #include <sys/epoll.h>
>> #include <sys/stat.h>
>>
>> +#include <stdarg.h>
>> #include <stdio.h>
>> #include <stdlib.h>
>> #include <string.h>
>> #include <execinfo.h>
>>
>> #include "ustcomm.h"
>> -#include "usterr.h"
>> +#include <ust/core.h>
>> #include "share.h"
>>
>> static int mkdir_p(const char *path, mode_t mode)
>> @@ -101,7 +102,6 @@ static struct sockaddr_un *  
>> create_sock_addr(const char *name,
>>
>> 	addr = malloc(alloc_size);
>> 	if (addr < 0) {
>
> Hrm, the original test is bogus there ? malloc returns NULL if alloc
> fails, not a negative value... ?

True... Could fix that in a seperate patch.
>
>> -		ERR("allocating addr failed");
>> 		return NULL;
>> 	}
>>
>> @@ -149,9 +149,7 @@ void ustcomm_del_sock(struct ustcomm_sock  
>> *sock, int keep_in_epoll)
>> {
>> 	cds_list_del(&sock->list);
>> 	if (!keep_in_epoll) {
>> -		if (epoll_ctl(sock->epoll_fd, EPOLL_CTL_DEL, sock->fd, NULL) ==  
>> -1) {
>> -			PERROR("epoll_ctl: failed to delete socket");
>> -		}
>> +		epoll_ctl(sock->epoll_fd, EPOLL_CTL_DEL, sock->fd, NULL);
>
> I think we should keep the error value test here ?

No, EPOLL_CTL_DEL can only return error ENONENT, if there was no fd  
with that fd number, in which cas everything anyway is fine.

>
>> 	}
>> 	close(sock->fd);
>> 	free(sock);
>> @@ -168,13 +166,11 @@ struct ustcomm_sock *  
>> ustcomm_init_named_socket(const char *name,
>>
>> 	fd = socket(PF_UNIX, SOCK_STREAM, 0);
>> 	if(fd == -1) {
>> -		PERROR("socket");
>> 		return NULL;
>> 	}
>>
>> 	addr = create_sock_addr(name, &sock_addr_size);
>> 	if (addr == NULL) {
>> -		ERR("allocating addr, UST thread bailing");
>> 		goto close_sock;
>> 	}
>>
>> @@ -182,29 +178,24 @@ struct ustcomm_sock *  
>> ustcomm_init_named_socket(const char *name,
>> 	if(result == 0) {
>> 		/* file exists */
>> 		result = unlink(name);
>> -		if(result == -1) {
>> -			PERROR("unlink of socket file");
>> +		if (result == -1) {
>> 			goto free_addr;
>> 		}
>> -		DBG("socket already exists; overwriting");
>> 	}
>>
>> 	result = bind(fd, (struct sockaddr *)addr, sock_addr_size);
>> 	if(result == -1) {
>> -		PERROR("bind");
>> 		goto free_addr;
>> 	}
>>
>> 	result = listen(fd, 1);
>> 	if(result == -1) {
>> -		PERROR("listen");
>> 		goto free_addr;
>> 	}
>>
>> 	sock = ustcomm_init_sock(fd, epoll_fd,
>> 				 NULL);
>> 	if (!sock) {
>> -		ERR("failed to create ustcomm_sock");
>> 		goto free_addr;
>> 	}
>>
>> @@ -236,39 +227,31 @@ void ustcomm_del_named_sock(struct  
>> ustcomm_sock *sock,
>> 		/* Get the socket name */
>> 		alloc_size = sizeof(dummy);
>> 		if (getsockname(fd, &dummy, (socklen_t *)&alloc_size) < 0) {
>> -			PERROR("getsockname failed");
>> 			goto del_sock;
>> 		}
>>
>> 		sockaddr = zmalloc(alloc_size);
>> 		if (!sockaddr) {
>> -			ERR("failed to allocate sockaddr");
>> 			goto del_sock;
>> 		}
>>
>> 		if (getsockname(fd, sockaddr, (socklen_t *)&alloc_size) < 0) {
>> -			PERROR("getsockname failed");
>> 			goto free_sockaddr;
>> 		}
>>
>> 		/* Destroy socket */
>> 		result = stat(sockaddr->sun_path, &st);
>> 		if(result < 0) {
>> -			PERROR("stat (%s)", sockaddr->sun_path);
>> 			goto free_sockaddr;
>> 		}
>>
>> 		/* Paranoid check before deleting. */
>> 		result = S_ISSOCK(st.st_mode);
>> 		if(!result) {
>> -			ERR("The socket we are about to delete is not a socket.");
>> 			goto free_sockaddr;
>> 		}
>>
>> -		result = unlink(sockaddr->sun_path);
>> -		if(result < 0) {
>> -			PERROR("unlink");
>> -		}
>> +		unlink(sockaddr->sun_path);
>
> Should we do something with the error here ? Why is it not treated (if
> there is a reason, we should add a comment)

hm, could add a comment but not much point. This is just cleanup after  
all, if it fails we can't do anything about it and it won't affect  
anything.

/Nils

>
> Thanks,
>
> Mathieu
>
>> 	}
>>
>> free_sockaddr:
>> @@ -297,7 +280,6 @@ int ustcomm_recv_alloc(int sock,
>> 		} else if (errno == EINTR) {
>> 			return -1;
>> 		} else if (result < 0) {
>> -			PERROR("recv");
>> 			return -1;
>> 		}
>> 		return 0;
>> @@ -326,7 +308,6 @@ int ustcomm_recv_alloc(int sock,
>> 	result = recvmsg(sock, &msg, MSG_WAITALL);
>> 	if (result < 0) {
>> 		free(*data);
>> -		PERROR("recvmsg failed");
>> 	}
>>
>> 	return result;
>> @@ -355,7 +336,6 @@ int ustcomm_recv_fd(int sock,
>> 		} else if (errno == EINTR) {
>> 			return -1;
>> 		} else if (result < 0) {
>> -			PERROR("recv");
>> 			return -1;
>> 		}
>> 		return 0;
>> @@ -372,7 +352,6 @@ int ustcomm_recv_fd(int sock,
>> 	if (peek_header.size && data) {
>> 		if (peek_header.size < 0 ||
>> 		    peek_header.size > USTCOMM_DATA_SIZE) {
>> -			ERR("big peek header! %ld", peek_header.size);
>> 			return 0;
>> 		}
>>
>> @@ -389,9 +368,6 @@ int ustcomm_recv_fd(int sock,
>>
>> 	result = recvmsg(sock, &msg, MSG_WAITALL);
>> 	if (result <= 0) {
>> -		if (result < 0) {
>> -			PERROR("recvmsg failed");
>> -		}
>> 		return result;
>> 	}
>>
>> @@ -407,9 +383,6 @@ int ustcomm_recv_fd(int sock,
>> 			}
>> 			cmsg = CMSG_NXTHDR(&msg, cmsg);
>> 		}
>> -		if (!result) {
>> -			ERR("Failed to receive file descriptor\n");
>> -		}
>> 	}
>>
>> 	return 1;
>> @@ -430,7 +403,6 @@ int ustcomm_send_fd(int sock,
>> {
>> 	struct iovec iov[2];
>> 	struct msghdr msg;
>> -	int result;
>> 	struct cmsghdr *cmsg;
>> 	char buf[CMSG_SPACE(sizeof(int))];
>>
>> @@ -461,11 +433,7 @@ int ustcomm_send_fd(int sock,
>> 		msg.msg_controllen = cmsg->cmsg_len;
>> 	}
>>
>> -	result = sendmsg(sock, &msg, MSG_NOSIGNAL);
>> -	if (result < 0 && errno != EPIPE) {
>> -		PERROR("sendmsg failed");
>> -	}
>> -	return result;
>> +	return sendmsg(sock, &msg, MSG_NOSIGNAL);
>> }
>>
>> int ustcomm_send(int sock,
>> @@ -504,19 +472,16 @@ int ustcomm_connect_path(const char *name,  
>> int *connection_fd)
>>
>> 	fd = socket(PF_UNIX, SOCK_STREAM, 0);
>> 	if(fd == -1) {
>> -		PERROR("socket");
>> 		return -1;
>> 	}
>>
>> 	addr = create_sock_addr(name, &sock_addr_size);
>> 	if (addr == NULL) {
>> -		ERR("allocating addr failed");
>> 		goto close_sock;
>> 	}
>>
>> 	result = connect(fd, (struct sockaddr *)addr, sock_addr_size);
>> 	if(result == -1) {
>> -		PERROR("connect (path=%s)", name);
>> 		goto free_sock_addr;
>> 	}
>>
>> @@ -543,7 +508,6 @@ char *ustcomm_user_sock_dir(void)
>> 	result = asprintf(&sock_dir, "%s%s", USER_SOCK_DIR,
>> 			  cuserid(NULL));
>> 	if (result < 0) {
>> -		ERR("string overflow allocating directory name");
>> 		return NULL;
>> 	}
>>
>> @@ -569,14 +533,12 @@ static int connect_app_non_root(pid_t pid,  
>> int *app_fd)
>>
>> 	result = asprintf(&sock_name, "%s/%d", dir_name, pid);
>> 	if (result < 0) {
>> -		ERR("failed to allocate socket name");
>> 		retval = -1;
>> 		goto free_dir_name;
>> 	}
>>
>> 	result = ustcomm_connect_path(sock_name, app_fd);
>> 	if (result < 0) {
>> -		ERR("failed to connect to app");
>> 		retval = -1;
>> 		goto free_sock_name;
>> 	}
>> @@ -657,14 +619,12 @@ int ensure_dir_exists(const char *dir, mode_t  
>> mode)
>>
>> 		result = mkdir_p(dir, mode);
>> 		if(result != 0) {
>> -			ERR("executing in recursive creation of directory %s", dir);
>> 			return -1;
>> 		}
>> 	} else {
>> 		if (st.st_mode != mode) {
>> 			result = chmod(dir, mode);
>> 			if (result < 0) {
>> -				ERR("couldn't set directory mode on %s", dir);
>> 				return -1;
>> 			}
>> 		}
>> diff --git a/libustctl/libustctl.c b/libustctl/libustctl.c
>> index 1911434..b5d6a02 100644
>> --- a/libustctl/libustctl.c
>> +++ b/libustctl/libustctl.c
>> @@ -17,6 +17,7 @@
>>  */
>>
>> #define _GNU_SOURCE
>> +#include <errno.h>
>> #include <stdio.h>
>> #include <unistd.h>
>> #include <getopt.h>
>> @@ -26,8 +27,8 @@
>> #include <dirent.h>
>>
>> #include "ustcomm.h"
>> -#include "ust/ustctl.h"
>> -#include "usterr.h"
>> +#include <ust/ustctl.h>
>> +#include <ust/core.h>
>>
>> static int do_cmd(int sock,
>> 		  const struct ustcomm_header *req_header,
>> @@ -57,7 +58,6 @@ static int do_cmd(int sock,
>> 			}
>> 		}
>> 	} else {
>> -		ERR("ustcomm req failed");
>> 		if (result == 0) {
>> 			saved_errno = ENOTCONN;
>> 		} else {
>> @@ -80,7 +80,6 @@ int ustctl_connect_pid(pid_t pid)
>> 	int sock;
>>
>> 	if (ustcomm_connect_app(pid, &sock)) {
>> -		ERR("could not connect to PID %u", (unsigned int) pid);
>> 		errno = ENOTCONN;
>> 		return -1;
>> 	}
>> @@ -580,20 +579,17 @@ int ustctl_get_cmsf(int sock, struct  
>> marker_status **cmsf)
>>
>> 	result = ustcomm_send(sock, &req_header, NULL);
>> 	if (result <= 0) {
>> -		PERROR("error while requesting markers list");
>> 		return -1;
>> 	}
>>
>> 	result = ustcomm_recv_alloc(sock, &res_header, &big_str);
>> 	if (result <= 0) {
>> -		ERR("error while receiving markers list");
>> 		return -1;
>> 	}
>>
>> 	tmp_cmsf = (struct marker_status *) zmalloc(sizeof(struct  
>> marker_status) *
>> 						    (ustctl_count_nl(big_str) + 1));
>> 	if (tmp_cmsf == NULL) {
>> -		ERR("Failed to allocate CMSF array");
>> 		return -1;
>> 	}
>>
>> @@ -672,13 +668,11 @@ int ustctl_get_tes(int sock, struct  
>> trace_event_status **tes)
>>
>> 	result = ustcomm_send(sock, &req_header, NULL);
>> 	if (result != 1) {
>> -		ERR("error while requesting trace_event list");
>> 		return -1;
>> 	}
>>
>> 	result = ustcomm_recv_alloc(sock, &res_header, &big_str);
>> 	if (result != 1) {
>> -		ERR("error while receiving markers list");
>> 		return -1;
>> 	}
>>
>> @@ -686,7 +680,6 @@ int ustctl_get_tes(int sock, struct  
>> trace_event_status **tes)
>> 		zmalloc(sizeof(struct trace_event_status) *
>> 			(ustctl_count_nl(big_str) + 1));
>> 	if (tmp_tes == NULL) {
>> -		ERR("Failed to allocate TES array");
>> 		return -1;
>> 	}
>>
>> -- 
>> 1.7.1
>>
>>
>> _______________________________________________
>> ltt-dev mailing list
>> ltt-dev at lists.casi.polymtl.ca
>> http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev
>>
>
> -- 
> Mathieu Desnoyers
> Operating System Efficiency R&D Consultant
> EfficiOS Inc.
> http://www.efficios.com
>
> _______________________________________________
> ltt-dev mailing list
> ltt-dev at lists.casi.polymtl.ca
> http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev

[ltt-dev] [UST PATCH] Remove all "usterr.h" references in libusctl/ustcomm

[Mathieu Desnoyers]

* Nils Carlson (nils.carlson at ludd.ltu.se) wrote:
>
> On Apr 1, 2011, at 5:14 PM, Mathieu Desnoyers wrote:
>
>> * Nils Carlson (nils.carlson at ericsson.com) wrote:
>>> This patch removes all references to usterr.h and functions in
>>> usterr.h from libustctl and ustcomm. This is done for three reasons:
>>>
>>> 1. DBG, ERR, PERROR etc call in ust_safe_snprintf, I don't know
>>>   why, and I can't figure out any good reasons. If somebody
>>>   can enlighten me I would be very grateful. The only thing
>>>   I can figure out is that these are some sort of "signal safe"
>>>   printf.
>>
>> Yep, that's right.
>>
>>>
>>> 2. All this stuff is quite stable and working and libraries really
>>>   shouldn't be printing stuff.
>>
>> What I disagree about the patch below is that you are removing debug
>> error printouts without taking care of some of the error values in  
>> many
>> cases (just ignoring them).
>>
>> So we start from a dubtious behavior (debug error messages was not the
>> proper way to handle error values; they should rather have been  
>> treated
>> and pushed to the callers), to just ignoring the errors, which is kind
>> of bad, you know... ;)
>>
>> So I would expect that all the error values you remove debug messages
>> from should be dealt with rather than ignored.
>
> Hm. errno's are still there... As long as errnos aren't overwritten they 
> can still be used. Can't find anywhere where we are making calls that 
> should overwrite errnos. We could try to introduce consistency in  
> returning negative errno's though. That would be a separate patch.
>>
>>>
>>> 3. This allows me to compile lttng-tools without linking libustctl
>>>   to the ust_snprintf family of functions. This makes me happy.
>>
>> Well, you could just create a set of debug macros that use the  
>> standard
>> printf, and decide at compile-time if the log output is built or not
>> with these pretty simple macros. This change would make more sense to 
>> me
>> than removing the debugging code we have there.
>>
> Well, could do that... but as noted, this is pretty stable stuff and we 
> can add debug code as needed I think.
>
>> More comments below,
>>
>>>
>>> Mathieu, enlighten me as best you can... :-)
>>>
>>> Signed-off-by: Nils Carlson <nils.carlson at ericsson.com>
>>> ---
>>> libustcomm/ustcomm.c  |   52 ++++ 
>>> +-------------------------------------------
>>> libustctl/libustctl.c |   13 ++---------
>>> 2 files changed, 9 insertions(+), 56 deletions(-)
>>>
>>> diff --git a/libustcomm/ustcomm.c b/libustcomm/ustcomm.c
>>> index dcf8cd8..2a5acc7 100644
>>> --- a/libustcomm/ustcomm.c
>>> +++ b/libustcomm/ustcomm.c
>>> @@ -29,13 +29,14 @@
>>> #include <sys/epoll.h>
>>> #include <sys/stat.h>
>>>
>>> +#include <stdarg.h>
>>> #include <stdio.h>
>>> #include <stdlib.h>
>>> #include <string.h>
>>> #include <execinfo.h>
>>>
>>> #include "ustcomm.h"
>>> -#include "usterr.h"
>>> +#include <ust/core.h>
>>> #include "share.h"
>>>
>>> static int mkdir_p(const char *path, mode_t mode)
>>> @@ -101,7 +102,6 @@ static struct sockaddr_un *  
>>> create_sock_addr(const char *name,
>>>
>>> 	addr = malloc(alloc_size);
>>> 	if (addr < 0) {
>>
>> Hrm, the original test is bogus there ? malloc returns NULL if alloc
>> fails, not a negative value... ?
>
> True... Could fix that in a seperate patch.
>>
>>> -		ERR("allocating addr failed");
>>> 		return NULL;
>>> 	}
>>>
>>> @@ -149,9 +149,7 @@ void ustcomm_del_sock(struct ustcomm_sock *sock, 
>>> int keep_in_epoll)
>>> {
>>> 	cds_list_del(&sock->list);
>>> 	if (!keep_in_epoll) {
>>> -		if (epoll_ctl(sock->epoll_fd, EPOLL_CTL_DEL, sock->fd, NULL) ==  
>>> -1) {
>>> -			PERROR("epoll_ctl: failed to delete socket");
>>> -		}
>>> +		epoll_ctl(sock->epoll_fd, EPOLL_CTL_DEL, sock->fd, NULL);
>>
>> I think we should keep the error value test here ?
>
> No, EPOLL_CTL_DEL can only return error ENONENT, if there was no fd with 
> that fd number, in which cas everything anyway is fine.

This is the kind of obvious safety net we should leave in place. It's
not because we expect that we'll always pass a valid FD that we won't
have a bug creeping in at some point, and checking the returned error
value is the kind of safety measure that will help us diagnose problems
much more quickly in the long run.

>
>>
>>> 	}
>>> 	close(sock->fd);
>>> 	free(sock);
>>> @@ -168,13 +166,11 @@ struct ustcomm_sock *  
>>> ustcomm_init_named_socket(const char *name,
>>>
>>> 	fd = socket(PF_UNIX, SOCK_STREAM, 0);
>>> 	if(fd == -1) {
>>> -		PERROR("socket");
>>> 		return NULL;
>>> 	}
>>>
>>> 	addr = create_sock_addr(name, &sock_addr_size);
>>> 	if (addr == NULL) {
>>> -		ERR("allocating addr, UST thread bailing");
>>> 		goto close_sock;
>>> 	}
>>>
>>> @@ -182,29 +178,24 @@ struct ustcomm_sock *  
>>> ustcomm_init_named_socket(const char *name,
>>> 	if(result == 0) {
>>> 		/* file exists */
>>> 		result = unlink(name);
>>> -		if(result == -1) {
>>> -			PERROR("unlink of socket file");
>>> +		if (result == -1) {
>>> 			goto free_addr;
>>> 		}
>>> -		DBG("socket already exists; overwriting");
>>> 	}
>>>
>>> 	result = bind(fd, (struct sockaddr *)addr, sock_addr_size);
>>> 	if(result == -1) {
>>> -		PERROR("bind");
>>> 		goto free_addr;
>>> 	}
>>>
>>> 	result = listen(fd, 1);
>>> 	if(result == -1) {
>>> -		PERROR("listen");
>>> 		goto free_addr;
>>> 	}
>>>
>>> 	sock = ustcomm_init_sock(fd, epoll_fd,
>>> 				 NULL);
>>> 	if (!sock) {
>>> -		ERR("failed to create ustcomm_sock");
>>> 		goto free_addr;
>>> 	}
>>>
>>> @@ -236,39 +227,31 @@ void ustcomm_del_named_sock(struct  
>>> ustcomm_sock *sock,
>>> 		/* Get the socket name */
>>> 		alloc_size = sizeof(dummy);
>>> 		if (getsockname(fd, &dummy, (socklen_t *)&alloc_size) < 0) {
>>> -			PERROR("getsockname failed");
>>> 			goto del_sock;
>>> 		}
>>>
>>> 		sockaddr = zmalloc(alloc_size);
>>> 		if (!sockaddr) {
>>> -			ERR("failed to allocate sockaddr");
>>> 			goto del_sock;
>>> 		}
>>>
>>> 		if (getsockname(fd, sockaddr, (socklen_t *)&alloc_size) < 0) {
>>> -			PERROR("getsockname failed");
>>> 			goto free_sockaddr;
>>> 		}
>>>
>>> 		/* Destroy socket */
>>> 		result = stat(sockaddr->sun_path, &st);
>>> 		if(result < 0) {
>>> -			PERROR("stat (%s)", sockaddr->sun_path);
>>> 			goto free_sockaddr;
>>> 		}
>>>
>>> 		/* Paranoid check before deleting. */
>>> 		result = S_ISSOCK(st.st_mode);
>>> 		if(!result) {
>>> -			ERR("The socket we are about to delete is not a socket.");
>>> 			goto free_sockaddr;
>>> 		}
>>>
>>> -		result = unlink(sockaddr->sun_path);
>>> -		if(result < 0) {
>>> -			PERROR("unlink");
>>> -		}
>>> +		unlink(sockaddr->sun_path);
>>
>> Should we do something with the error here ? Why is it not treated (if
>> there is a reason, we should add a comment)
>
> hm, could add a comment but not much point. This is just cleanup after  
> all, if it fails we can't do anything about it and it won't affect  
> anything.

I recommend this to ensure that all return values are either

- treated, or
- documented.

looking at unlink(2), there are many reasons why unlink can fail. If it
fails due to incorrect access rights (for instance), we probably want to
propagate the error up to the user, no ?

Thanks,

Mathieu

>
> /Nils
>
>>
>> Thanks,
>>
>> Mathieu
>>
>>> 	}
>>>
>>> free_sockaddr:
>>> @@ -297,7 +280,6 @@ int ustcomm_recv_alloc(int sock,
>>> 		} else if (errno == EINTR) {
>>> 			return -1;
>>> 		} else if (result < 0) {
>>> -			PERROR("recv");
>>> 			return -1;
>>> 		}
>>> 		return 0;
>>> @@ -326,7 +308,6 @@ int ustcomm_recv_alloc(int sock,
>>> 	result = recvmsg(sock, &msg, MSG_WAITALL);
>>> 	if (result < 0) {
>>> 		free(*data);
>>> -		PERROR("recvmsg failed");
>>> 	}
>>>
>>> 	return result;
>>> @@ -355,7 +336,6 @@ int ustcomm_recv_fd(int sock,
>>> 		} else if (errno == EINTR) {
>>> 			return -1;
>>> 		} else if (result < 0) {
>>> -			PERROR("recv");
>>> 			return -1;
>>> 		}
>>> 		return 0;
>>> @@ -372,7 +352,6 @@ int ustcomm_recv_fd(int sock,
>>> 	if (peek_header.size && data) {
>>> 		if (peek_header.size < 0 ||
>>> 		    peek_header.size > USTCOMM_DATA_SIZE) {
>>> -			ERR("big peek header! %ld", peek_header.size);
>>> 			return 0;
>>> 		}
>>>
>>> @@ -389,9 +368,6 @@ int ustcomm_recv_fd(int sock,
>>>
>>> 	result = recvmsg(sock, &msg, MSG_WAITALL);
>>> 	if (result <= 0) {
>>> -		if (result < 0) {
>>> -			PERROR("recvmsg failed");
>>> -		}
>>> 		return result;
>>> 	}
>>>
>>> @@ -407,9 +383,6 @@ int ustcomm_recv_fd(int sock,
>>> 			}
>>> 			cmsg = CMSG_NXTHDR(&msg, cmsg);
>>> 		}
>>> -		if (!result) {
>>> -			ERR("Failed to receive file descriptor\n");
>>> -		}
>>> 	}
>>>
>>> 	return 1;
>>> @@ -430,7 +403,6 @@ int ustcomm_send_fd(int sock,
>>> {
>>> 	struct iovec iov[2];
>>> 	struct msghdr msg;
>>> -	int result;
>>> 	struct cmsghdr *cmsg;
>>> 	char buf[CMSG_SPACE(sizeof(int))];
>>>
>>> @@ -461,11 +433,7 @@ int ustcomm_send_fd(int sock,
>>> 		msg.msg_controllen = cmsg->cmsg_len;
>>> 	}
>>>
>>> -	result = sendmsg(sock, &msg, MSG_NOSIGNAL);
>>> -	if (result < 0 && errno != EPIPE) {
>>> -		PERROR("sendmsg failed");
>>> -	}
>>> -	return result;
>>> +	return sendmsg(sock, &msg, MSG_NOSIGNAL);
>>> }
>>>
>>> int ustcomm_send(int sock,
>>> @@ -504,19 +472,16 @@ int ustcomm_connect_path(const char *name, int 
>>> *connection_fd)
>>>
>>> 	fd = socket(PF_UNIX, SOCK_STREAM, 0);
>>> 	if(fd == -1) {
>>> -		PERROR("socket");
>>> 		return -1;
>>> 	}
>>>
>>> 	addr = create_sock_addr(name, &sock_addr_size);
>>> 	if (addr == NULL) {
>>> -		ERR("allocating addr failed");
>>> 		goto close_sock;
>>> 	}
>>>
>>> 	result = connect(fd, (struct sockaddr *)addr, sock_addr_size);
>>> 	if(result == -1) {
>>> -		PERROR("connect (path=%s)", name);
>>> 		goto free_sock_addr;
>>> 	}
>>>
>>> @@ -543,7 +508,6 @@ char *ustcomm_user_sock_dir(void)
>>> 	result = asprintf(&sock_dir, "%s%s", USER_SOCK_DIR,
>>> 			  cuserid(NULL));
>>> 	if (result < 0) {
>>> -		ERR("string overflow allocating directory name");
>>> 		return NULL;
>>> 	}
>>>
>>> @@ -569,14 +533,12 @@ static int connect_app_non_root(pid_t pid, int 
>>> *app_fd)
>>>
>>> 	result = asprintf(&sock_name, "%s/%d", dir_name, pid);
>>> 	if (result < 0) {
>>> -		ERR("failed to allocate socket name");
>>> 		retval = -1;
>>> 		goto free_dir_name;
>>> 	}
>>>
>>> 	result = ustcomm_connect_path(sock_name, app_fd);
>>> 	if (result < 0) {
>>> -		ERR("failed to connect to app");
>>> 		retval = -1;
>>> 		goto free_sock_name;
>>> 	}
>>> @@ -657,14 +619,12 @@ int ensure_dir_exists(const char *dir, mode_t  
>>> mode)
>>>
>>> 		result = mkdir_p(dir, mode);
>>> 		if(result != 0) {
>>> -			ERR("executing in recursive creation of directory %s", dir);
>>> 			return -1;
>>> 		}
>>> 	} else {
>>> 		if (st.st_mode != mode) {
>>> 			result = chmod(dir, mode);
>>> 			if (result < 0) {
>>> -				ERR("couldn't set directory mode on %s", dir);
>>> 				return -1;
>>> 			}
>>> 		}
>>> diff --git a/libustctl/libustctl.c b/libustctl/libustctl.c
>>> index 1911434..b5d6a02 100644
>>> --- a/libustctl/libustctl.c
>>> +++ b/libustctl/libustctl.c
>>> @@ -17,6 +17,7 @@
>>>  */
>>>
>>> #define _GNU_SOURCE
>>> +#include <errno.h>
>>> #include <stdio.h>
>>> #include <unistd.h>
>>> #include <getopt.h>
>>> @@ -26,8 +27,8 @@
>>> #include <dirent.h>
>>>
>>> #include "ustcomm.h"
>>> -#include "ust/ustctl.h"
>>> -#include "usterr.h"
>>> +#include <ust/ustctl.h>
>>> +#include <ust/core.h>
>>>
>>> static int do_cmd(int sock,
>>> 		  const struct ustcomm_header *req_header,
>>> @@ -57,7 +58,6 @@ static int do_cmd(int sock,
>>> 			}
>>> 		}
>>> 	} else {
>>> -		ERR("ustcomm req failed");
>>> 		if (result == 0) {
>>> 			saved_errno = ENOTCONN;
>>> 		} else {
>>> @@ -80,7 +80,6 @@ int ustctl_connect_pid(pid_t pid)
>>> 	int sock;
>>>
>>> 	if (ustcomm_connect_app(pid, &sock)) {
>>> -		ERR("could not connect to PID %u", (unsigned int) pid);
>>> 		errno = ENOTCONN;
>>> 		return -1;
>>> 	}
>>> @@ -580,20 +579,17 @@ int ustctl_get_cmsf(int sock, struct  
>>> marker_status **cmsf)
>>>
>>> 	result = ustcomm_send(sock, &req_header, NULL);
>>> 	if (result <= 0) {
>>> -		PERROR("error while requesting markers list");
>>> 		return -1;
>>> 	}
>>>
>>> 	result = ustcomm_recv_alloc(sock, &res_header, &big_str);
>>> 	if (result <= 0) {
>>> -		ERR("error while receiving markers list");
>>> 		return -1;
>>> 	}
>>>
>>> 	tmp_cmsf = (struct marker_status *) zmalloc(sizeof(struct  
>>> marker_status) *
>>> 						    (ustctl_count_nl(big_str) + 1));
>>> 	if (tmp_cmsf == NULL) {
>>> -		ERR("Failed to allocate CMSF array");
>>> 		return -1;
>>> 	}
>>>
>>> @@ -672,13 +668,11 @@ int ustctl_get_tes(int sock, struct  
>>> trace_event_status **tes)
>>>
>>> 	result = ustcomm_send(sock, &req_header, NULL);
>>> 	if (result != 1) {
>>> -		ERR("error while requesting trace_event list");
>>> 		return -1;
>>> 	}
>>>
>>> 	result = ustcomm_recv_alloc(sock, &res_header, &big_str);
>>> 	if (result != 1) {
>>> -		ERR("error while receiving markers list");
>>> 		return -1;
>>> 	}
>>>
>>> @@ -686,7 +680,6 @@ int ustctl_get_tes(int sock, struct  
>>> trace_event_status **tes)
>>> 		zmalloc(sizeof(struct trace_event_status) *
>>> 			(ustctl_count_nl(big_str) + 1));
>>> 	if (tmp_tes == NULL) {
>>> -		ERR("Failed to allocate TES array");
>>> 		return -1;
>>> 	}
>>>
>>> -- 
>>> 1.7.1
>>>
>>>
>>> _______________________________________________
>>> ltt-dev mailing list
>>> ltt-dev at lists.casi.polymtl.ca
>>> http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev
>>>
>>
>> -- 
>> Mathieu Desnoyers
>> Operating System Efficiency R&D Consultant
>> EfficiOS Inc.
>> http://www.efficios.com
>>
>> _______________________________________________
>> ltt-dev mailing list
>> ltt-dev at lists.casi.polymtl.ca
>> http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev
>

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

[ltt-dev] [UST PATCH] Remove all "usterr.h" references in libusctl/ustcomm

[Nils Carlson]

On Apr 1, 2011, at 10:24 PM, Mathieu Desnoyers wrote:

> * Nils Carlson (nils.carlson at ludd.ltu.se) wrote:
>>
>> On Apr 1, 2011, at 5:14 PM, Mathieu Desnoyers wrote:
>>
>>> * Nils Carlson (nils.carlson at ericsson.com) wrote:
>>>> This patch removes all references to usterr.h and functions in
>>>> usterr.h from libustctl and ustcomm. This is done for three  
>>>> reasons:
>>>>
>>>> 1. DBG, ERR, PERROR etc call in ust_safe_snprintf, I don't know
>>>>  why, and I can't figure out any good reasons. If somebody
>>>>  can enlighten me I would be very grateful. The only thing
>>>>  I can figure out is that these are some sort of "signal safe"
>>>>  printf.
>>>
>>> Yep, that's right.
>>>
>>>>
>>>> 2. All this stuff is quite stable and working and libraries really
>>>>  shouldn't be printing stuff.
>>>
>>> What I disagree about the patch below is that you are removing debug
>>> error printouts without taking care of some of the error values in
>>> many
>>> cases (just ignoring them).
>>>
>>> So we start from a dubtious behavior (debug error messages was not  
>>> the
>>> proper way to handle error values; they should rather have been
>>> treated
>>> and pushed to the callers), to just ignoring the errors, which is  
>>> kind
>>> of bad, you know... ;)
>>>
>>> So I would expect that all the error values you remove debug  
>>> messages
>>> from should be dealt with rather than ignored.
>>
>> Hm. errno's are still there... As long as errnos aren't overwritten  
>> they
>> can still be used. Can't find anywhere where we are making calls that
>> should overwrite errnos. We could try to introduce consistency in
>> returning negative errno's though. That would be a separate patch.
>>>
>>>>
>>>> 3. This allows me to compile lttng-tools without linking libustctl
>>>>  to the ust_snprintf family of functions. This makes me happy.
>>>
>>> Well, you could just create a set of debug macros that use the
>>> standard
>>> printf, and decide at compile-time if the log output is built or not
>>> with these pretty simple macros. This change would make more sense  
>>> to
>>> me
>>> than removing the debugging code we have there.
>>>
>> Well, could do that... but as noted, this is pretty stable stuff  
>> and we
>> can add debug code as needed I think.
>>
>>> More comments below,
>>>
>>>>
>>>> Mathieu, enlighten me as best you can... :-)
>>>>
>>>> Signed-off-by: Nils Carlson <nils.carlson at ericsson.com>
>>>> ---
>>>> libustcomm/ustcomm.c  |   52 ++++
>>>> +-------------------------------------------
>>>> libustctl/libustctl.c |   13 ++---------
>>>> 2 files changed, 9 insertions(+), 56 deletions(-)
>>>>
>>>> diff --git a/libustcomm/ustcomm.c b/libustcomm/ustcomm.c
>>>> index dcf8cd8..2a5acc7 100644
>>>> --- a/libustcomm/ustcomm.c
>>>> +++ b/libustcomm/ustcomm.c
>>>> @@ -29,13 +29,14 @@
>>>> #include <sys/epoll.h>
>>>> #include <sys/stat.h>
>>>>
>>>> +#include <stdarg.h>
>>>> #include <stdio.h>
>>>> #include <stdlib.h>
>>>> #include <string.h>
>>>> #include <execinfo.h>
>>>>
>>>> #include "ustcomm.h"
>>>> -#include "usterr.h"
>>>> +#include <ust/core.h>
>>>> #include "share.h"
>>>>
>>>> static int mkdir_p(const char *path, mode_t mode)
>>>> @@ -101,7 +102,6 @@ static struct sockaddr_un *
>>>> create_sock_addr(const char *name,
>>>>
>>>> 	addr = malloc(alloc_size);
>>>> 	if (addr < 0) {
>>>
>>> Hrm, the original test is bogus there ? malloc returns NULL if alloc
>>> fails, not a negative value... ?
>>
>> True... Could fix that in a seperate patch.
>>>
>>>> -		ERR("allocating addr failed");
>>>> 		return NULL;
>>>> 	}
>>>>
>>>> @@ -149,9 +149,7 @@ void ustcomm_del_sock(struct ustcomm_sock  
>>>> *sock,
>>>> int keep_in_epoll)
>>>> {
>>>> 	cds_list_del(&sock->list);
>>>> 	if (!keep_in_epoll) {
>>>> -		if (epoll_ctl(sock->epoll_fd, EPOLL_CTL_DEL, sock->fd, NULL) ==
>>>> -1) {
>>>> -			PERROR("epoll_ctl: failed to delete socket");
>>>> -		}
>>>> +		epoll_ctl(sock->epoll_fd, EPOLL_CTL_DEL, sock->fd, NULL);
>>>
>>> I think we should keep the error value test here ?
>>
>> No, EPOLL_CTL_DEL can only return error ENONENT, if there was no fd  
>> with
>> that fd number, in which cas everything anyway is fine.
>
> This is the kind of obvious safety net we should leave in place. It's
> not because we expect that we'll always pass a valid FD that we won't
> have a bug creeping in at some point, and checking the returned error
> value is the kind of safety measure that will help us diagnose  
> problems
> much more quickly in the long run.
hm. Remember, this is a tracing library. It should be quiet.

What do you suggest we do with the error? the function doesn't return  
anything.
The only thing we can do is spew out a error message, which if we are  
lucky may be reported, probably be ignored, and in worst case screw up  
a program using stdout/stderr for something.

Maybe we should discuss this though. What should UST do/not-do. Should  
UST ever throw stuff out on stdout/stderr?

>
>>
>>>
>>>> 	}
>>>> 	close(sock->fd);
>>>> 	free(sock);
>>>> @@ -168,13 +166,11 @@ struct ustcomm_sock *
>>>> ustcomm_init_named_socket(const char *name,
>>>>
>>>> 	fd = socket(PF_UNIX, SOCK_STREAM, 0);
>>>> 	if(fd == -1) {
>>>> -		PERROR("socket");
>>>> 		return NULL;
>>>> 	}
>>>>
>>>> 	addr = create_sock_addr(name, &sock_addr_size);
>>>> 	if (addr == NULL) {
>>>> -		ERR("allocating addr, UST thread bailing");
>>>> 		goto close_sock;
>>>> 	}
>>>>
>>>> @@ -182,29 +178,24 @@ struct ustcomm_sock *
>>>> ustcomm_init_named_socket(const char *name,
>>>> 	if(result == 0) {
>>>> 		/* file exists */
>>>> 		result = unlink(name);
>>>> -		if(result == -1) {
>>>> -			PERROR("unlink of socket file");
>>>> +		if (result == -1) {
>>>> 			goto free_addr;
>>>> 		}
>>>> -		DBG("socket already exists; overwriting");
>>>> 	}
>>>>
>>>> 	result = bind(fd, (struct sockaddr *)addr, sock_addr_size);
>>>> 	if(result == -1) {
>>>> -		PERROR("bind");
>>>> 		goto free_addr;
>>>> 	}
>>>>
>>>> 	result = listen(fd, 1);
>>>> 	if(result == -1) {
>>>> -		PERROR("listen");
>>>> 		goto free_addr;
>>>> 	}
>>>>
>>>> 	sock = ustcomm_init_sock(fd, epoll_fd,
>>>> 				 NULL);
>>>> 	if (!sock) {
>>>> -		ERR("failed to create ustcomm_sock");
>>>> 		goto free_addr;
>>>> 	}
>>>>
>>>> @@ -236,39 +227,31 @@ void ustcomm_del_named_sock(struct
>>>> ustcomm_sock *sock,
>>>> 		/* Get the socket name */
>>>> 		alloc_size = sizeof(dummy);
>>>> 		if (getsockname(fd, &dummy, (socklen_t *)&alloc_size) < 0) {
>>>> -			PERROR("getsockname failed");
>>>> 			goto del_sock;
>>>> 		}
>>>>
>>>> 		sockaddr = zmalloc(alloc_size);
>>>> 		if (!sockaddr) {
>>>> -			ERR("failed to allocate sockaddr");
>>>> 			goto del_sock;
>>>> 		}
>>>>
>>>> 		if (getsockname(fd, sockaddr, (socklen_t *)&alloc_size) < 0) {
>>>> -			PERROR("getsockname failed");
>>>> 			goto free_sockaddr;
>>>> 		}
>>>>
>>>> 		/* Destroy socket */
>>>> 		result = stat(sockaddr->sun_path, &st);
>>>> 		if(result < 0) {
>>>> -			PERROR("stat (%s)", sockaddr->sun_path);
>>>> 			goto free_sockaddr;
>>>> 		}
>>>>
>>>> 		/* Paranoid check before deleting. */
>>>> 		result = S_ISSOCK(st.st_mode);
>>>> 		if(!result) {
>>>> -			ERR("The socket we are about to delete is not a socket.");
>>>> 			goto free_sockaddr;
>>>> 		}
>>>>
>>>> -		result = unlink(sockaddr->sun_path);
>>>> -		if(result < 0) {
>>>> -			PERROR("unlink");
>>>> -		}
>>>> +		unlink(sockaddr->sun_path);
>>>
>>> Should we do something with the error here ? Why is it not treated  
>>> (if
>>> there is a reason, we should add a comment)
>>
>> hm, could add a comment but not much point. This is just cleanup  
>> after
>> all, if it fails we can't do anything about it and it won't affect
>> anything.
>
> I recommend this to ensure that all return values are either
>
> - treated, or
> - documented.
>
> looking at unlink(2), there are many reasons why unlink can fail. If  
> it
> fails due to incorrect access rights (for instance), we probably  
> want to
> propagate the error up to the user, no ?

For what? Nothing the user can/should do anyway.

/Nils
>
> Thanks,
>
> Mathieu
>
>>
>> /Nils
>>
>>>
>>> Thanks,
>>>
>>> Mathieu
>>>
>>>> 	}
>>>>
>>>> free_sockaddr:
>>>> @@ -297,7 +280,6 @@ int ustcomm_recv_alloc(int sock,
>>>> 		} else if (errno == EINTR) {
>>>> 			return -1;
>>>> 		} else if (result < 0) {
>>>> -			PERROR("recv");
>>>> 			return -1;
>>>> 		}
>>>> 		return 0;
>>>> @@ -326,7 +308,6 @@ int ustcomm_recv_alloc(int sock,
>>>> 	result = recvmsg(sock, &msg, MSG_WAITALL);
>>>> 	if (result < 0) {
>>>> 		free(*data);
>>>> -		PERROR("recvmsg failed");
>>>> 	}
>>>>
>>>> 	return result;
>>>> @@ -355,7 +336,6 @@ int ustcomm_recv_fd(int sock,
>>>> 		} else if (errno == EINTR) {
>>>> 			return -1;
>>>> 		} else if (result < 0) {
>>>> -			PERROR("recv");
>>>> 			return -1;
>>>> 		}
>>>> 		return 0;
>>>> @@ -372,7 +352,6 @@ int ustcomm_recv_fd(int sock,
>>>> 	if (peek_header.size && data) {
>>>> 		if (peek_header.size < 0 ||
>>>> 		    peek_header.size > USTCOMM_DATA_SIZE) {
>>>> -			ERR("big peek header! %ld", peek_header.size);
>>>> 			return 0;
>>>> 		}
>>>>
>>>> @@ -389,9 +368,6 @@ int ustcomm_recv_fd(int sock,
>>>>
>>>> 	result = recvmsg(sock, &msg, MSG_WAITALL);
>>>> 	if (result <= 0) {
>>>> -		if (result < 0) {
>>>> -			PERROR("recvmsg failed");
>>>> -		}
>>>> 		return result;
>>>> 	}
>>>>
>>>> @@ -407,9 +383,6 @@ int ustcomm_recv_fd(int sock,
>>>> 			}
>>>> 			cmsg = CMSG_NXTHDR(&msg, cmsg);
>>>> 		}
>>>> -		if (!result) {
>>>> -			ERR("Failed to receive file descriptor\n");
>>>> -		}
>>>> 	}
>>>>
>>>> 	return 1;
>>>> @@ -430,7 +403,6 @@ int ustcomm_send_fd(int sock,
>>>> {
>>>> 	struct iovec iov[2];
>>>> 	struct msghdr msg;
>>>> -	int result;
>>>> 	struct cmsghdr *cmsg;
>>>> 	char buf[CMSG_SPACE(sizeof(int))];
>>>>
>>>> @@ -461,11 +433,7 @@ int ustcomm_send_fd(int sock,
>>>> 		msg.msg_controllen = cmsg->cmsg_len;
>>>> 	}
>>>>
>>>> -	result = sendmsg(sock, &msg, MSG_NOSIGNAL);
>>>> -	if (result < 0 && errno != EPIPE) {
>>>> -		PERROR("sendmsg failed");
>>>> -	}
>>>> -	return result;
>>>> +	return sendmsg(sock, &msg, MSG_NOSIGNAL);
>>>> }
>>>>
>>>> int ustcomm_send(int sock,
>>>> @@ -504,19 +472,16 @@ int ustcomm_connect_path(const char *name,  
>>>> int
>>>> *connection_fd)
>>>>
>>>> 	fd = socket(PF_UNIX, SOCK_STREAM, 0);
>>>> 	if(fd == -1) {
>>>> -		PERROR("socket");
>>>> 		return -1;
>>>> 	}
>>>>
>>>> 	addr = create_sock_addr(name, &sock_addr_size);
>>>> 	if (addr == NULL) {
>>>> -		ERR("allocating addr failed");
>>>> 		goto close_sock;
>>>> 	}
>>>>
>>>> 	result = connect(fd, (struct sockaddr *)addr, sock_addr_size);
>>>> 	if(result == -1) {
>>>> -		PERROR("connect (path=%s)", name);
>>>> 		goto free_sock_addr;
>>>> 	}
>>>>
>>>> @@ -543,7 +508,6 @@ char *ustcomm_user_sock_dir(void)
>>>> 	result = asprintf(&sock_dir, "%s%s", USER_SOCK_DIR,
>>>> 			  cuserid(NULL));
>>>> 	if (result < 0) {
>>>> -		ERR("string overflow allocating directory name");
>>>> 		return NULL;
>>>> 	}
>>>>
>>>> @@ -569,14 +533,12 @@ static int connect_app_non_root(pid_t pid,  
>>>> int
>>>> *app_fd)
>>>>
>>>> 	result = asprintf(&sock_name, "%s/%d", dir_name, pid);
>>>> 	if (result < 0) {
>>>> -		ERR("failed to allocate socket name");
>>>> 		retval = -1;
>>>> 		goto free_dir_name;
>>>> 	}
>>>>
>>>> 	result = ustcomm_connect_path(sock_name, app_fd);
>>>> 	if (result < 0) {
>>>> -		ERR("failed to connect to app");
>>>> 		retval = -1;
>>>> 		goto free_sock_name;
>>>> 	}
>>>> @@ -657,14 +619,12 @@ int ensure_dir_exists(const char *dir, mode_t
>>>> mode)
>>>>
>>>> 		result = mkdir_p(dir, mode);
>>>> 		if(result != 0) {
>>>> -			ERR("executing in recursive creation of directory %s", dir);
>>>> 			return -1;
>>>> 		}
>>>> 	} else {
>>>> 		if (st.st_mode != mode) {
>>>> 			result = chmod(dir, mode);
>>>> 			if (result < 0) {
>>>> -				ERR("couldn't set directory mode on %s", dir);
>>>> 				return -1;
>>>> 			}
>>>> 		}
>>>> diff --git a/libustctl/libustctl.c b/libustctl/libustctl.c
>>>> index 1911434..b5d6a02 100644
>>>> --- a/libustctl/libustctl.c
>>>> +++ b/libustctl/libustctl.c
>>>> @@ -17,6 +17,7 @@
>>>> */
>>>>
>>>> #define _GNU_SOURCE
>>>> +#include <errno.h>
>>>> #include <stdio.h>
>>>> #include <unistd.h>
>>>> #include <getopt.h>
>>>> @@ -26,8 +27,8 @@
>>>> #include <dirent.h>
>>>>
>>>> #include "ustcomm.h"
>>>> -#include "ust/ustctl.h"
>>>> -#include "usterr.h"
>>>> +#include <ust/ustctl.h>
>>>> +#include <ust/core.h>
>>>>
>>>> static int do_cmd(int sock,
>>>> 		  const struct ustcomm_header *req_header,
>>>> @@ -57,7 +58,6 @@ static int do_cmd(int sock,
>>>> 			}
>>>> 		}
>>>> 	} else {
>>>> -		ERR("ustcomm req failed");
>>>> 		if (result == 0) {
>>>> 			saved_errno = ENOTCONN;
>>>> 		} else {
>>>> @@ -80,7 +80,6 @@ int ustctl_connect_pid(pid_t pid)
>>>> 	int sock;
>>>>
>>>> 	if (ustcomm_connect_app(pid, &sock)) {
>>>> -		ERR("could not connect to PID %u", (unsigned int) pid);
>>>> 		errno = ENOTCONN;
>>>> 		return -1;
>>>> 	}
>>>> @@ -580,20 +579,17 @@ int ustctl_get_cmsf(int sock, struct
>>>> marker_status **cmsf)
>>>>
>>>> 	result = ustcomm_send(sock, &req_header, NULL);
>>>> 	if (result <= 0) {
>>>> -		PERROR("error while requesting markers list");
>>>> 		return -1;
>>>> 	}
>>>>
>>>> 	result = ustcomm_recv_alloc(sock, &res_header, &big_str);
>>>> 	if (result <= 0) {
>>>> -		ERR("error while receiving markers list");
>>>> 		return -1;
>>>> 	}
>>>>
>>>> 	tmp_cmsf = (struct marker_status *) zmalloc(sizeof(struct
>>>> marker_status) *
>>>> 						    (ustctl_count_nl(big_str) + 1));
>>>> 	if (tmp_cmsf == NULL) {
>>>> -		ERR("Failed to allocate CMSF array");
>>>> 		return -1;
>>>> 	}
>>>>
>>>> @@ -672,13 +668,11 @@ int ustctl_get_tes(int sock, struct
>>>> trace_event_status **tes)
>>>>
>>>> 	result = ustcomm_send(sock, &req_header, NULL);
>>>> 	if (result != 1) {
>>>> -		ERR("error while requesting trace_event list");
>>>> 		return -1;
>>>> 	}
>>>>
>>>> 	result = ustcomm_recv_alloc(sock, &res_header, &big_str);
>>>> 	if (result != 1) {
>>>> -		ERR("error while receiving markers list");
>>>> 		return -1;
>>>> 	}
>>>>
>>>> @@ -686,7 +680,6 @@ int ustctl_get_tes(int sock, struct
>>>> trace_event_status **tes)
>>>> 		zmalloc(sizeof(struct trace_event_status) *
>>>> 			(ustctl_count_nl(big_str) + 1));
>>>> 	if (tmp_tes == NULL) {
>>>> -		ERR("Failed to allocate TES array");
>>>> 		return -1;
>>>> 	}
>>>>
>>>> -- 
>>>> 1.7.1
>>>>
>>>>
>>>> _______________________________________________
>>>> ltt-dev mailing list
>>>> ltt-dev at lists.casi.polymtl.ca
>>>> http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev
>>>>
>>>
>>> -- 
>>> Mathieu Desnoyers
>>> Operating System Efficiency R&D Consultant
>>> EfficiOS Inc.
>>> http://www.efficios.com
>>>
>>> _______________________________________________
>>> ltt-dev mailing list
>>> ltt-dev at lists.casi.polymtl.ca
>>> http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev
>>
>
> -- 
> Mathieu Desnoyers
> Operating System Efficiency R&D Consultant
> EfficiOS Inc.
> http://www.efficios.com

[ltt-dev] [UST PATCH] Remove all "usterr.h" references in libusctl/ustcomm

[Mathieu Desnoyers]

* Nils Carlson (nils.carlson at ludd.ltu.se) wrote:
>
> On Apr 1, 2011, at 10:24 PM, Mathieu Desnoyers wrote:
>
>> * Nils Carlson (nils.carlson at ludd.ltu.se) wrote:
>>>
>>> On Apr 1, 2011, at 5:14 PM, Mathieu Desnoyers wrote:
>>>
>>>> * Nils Carlson (nils.carlson at ericsson.com) wrote:
>>>>> This patch removes all references to usterr.h and functions in
>>>>> usterr.h from libustctl and ustcomm. This is done for three  
>>>>> reasons:
>>>>>
>>>>> 1. DBG, ERR, PERROR etc call in ust_safe_snprintf, I don't know
>>>>>  why, and I can't figure out any good reasons. If somebody
>>>>>  can enlighten me I would be very grateful. The only thing
>>>>>  I can figure out is that these are some sort of "signal safe"
>>>>>  printf.
>>>>
>>>> Yep, that's right.
>>>>
>>>>>
>>>>> 2. All this stuff is quite stable and working and libraries really
>>>>>  shouldn't be printing stuff.
>>>>
>>>> What I disagree about the patch below is that you are removing debug
>>>> error printouts without taking care of some of the error values in
>>>> many
>>>> cases (just ignoring them).
>>>>
>>>> So we start from a dubtious behavior (debug error messages was not  
>>>> the
>>>> proper way to handle error values; they should rather have been
>>>> treated
>>>> and pushed to the callers), to just ignoring the errors, which is  
>>>> kind
>>>> of bad, you know... ;)
>>>>
>>>> So I would expect that all the error values you remove debug  
>>>> messages
>>>> from should be dealt with rather than ignored.
>>>
>>> Hm. errno's are still there... As long as errnos aren't overwritten  
>>> they
>>> can still be used. Can't find anywhere where we are making calls that
>>> should overwrite errnos. We could try to introduce consistency in
>>> returning negative errno's though. That would be a separate patch.
>>>>
>>>>>
>>>>> 3. This allows me to compile lttng-tools without linking libustctl
>>>>>  to the ust_snprintf family of functions. This makes me happy.
>>>>
>>>> Well, you could just create a set of debug macros that use the
>>>> standard
>>>> printf, and decide at compile-time if the log output is built or not
>>>> with these pretty simple macros. This change would make more sense  
>>>> to
>>>> me
>>>> than removing the debugging code we have there.
>>>>
>>> Well, could do that... but as noted, this is pretty stable stuff and 
>>> we
>>> can add debug code as needed I think.
>>>
>>>> More comments below,
>>>>
>>>>>
>>>>> Mathieu, enlighten me as best you can... :-)
>>>>>
>>>>> Signed-off-by: Nils Carlson <nils.carlson at ericsson.com>
>>>>> ---
>>>>> libustcomm/ustcomm.c  |   52 ++++
>>>>> +-------------------------------------------
>>>>> libustctl/libustctl.c |   13 ++---------
>>>>> 2 files changed, 9 insertions(+), 56 deletions(-)
>>>>>
>>>>> diff --git a/libustcomm/ustcomm.c b/libustcomm/ustcomm.c
>>>>> index dcf8cd8..2a5acc7 100644
>>>>> --- a/libustcomm/ustcomm.c
>>>>> +++ b/libustcomm/ustcomm.c
>>>>> @@ -29,13 +29,14 @@
>>>>> #include <sys/epoll.h>
>>>>> #include <sys/stat.h>
>>>>>
>>>>> +#include <stdarg.h>
>>>>> #include <stdio.h>
>>>>> #include <stdlib.h>
>>>>> #include <string.h>
>>>>> #include <execinfo.h>
>>>>>
>>>>> #include "ustcomm.h"
>>>>> -#include "usterr.h"
>>>>> +#include <ust/core.h>
>>>>> #include "share.h"
>>>>>
>>>>> static int mkdir_p(const char *path, mode_t mode)
>>>>> @@ -101,7 +102,6 @@ static struct sockaddr_un *
>>>>> create_sock_addr(const char *name,
>>>>>
>>>>> 	addr = malloc(alloc_size);
>>>>> 	if (addr < 0) {
>>>>
>>>> Hrm, the original test is bogus there ? malloc returns NULL if alloc
>>>> fails, not a negative value... ?
>>>
>>> True... Could fix that in a seperate patch.
>>>>
>>>>> -		ERR("allocating addr failed");
>>>>> 		return NULL;
>>>>> 	}
>>>>>
>>>>> @@ -149,9 +149,7 @@ void ustcomm_del_sock(struct ustcomm_sock  
>>>>> *sock,
>>>>> int keep_in_epoll)
>>>>> {
>>>>> 	cds_list_del(&sock->list);
>>>>> 	if (!keep_in_epoll) {
>>>>> -		if (epoll_ctl(sock->epoll_fd, EPOLL_CTL_DEL, sock->fd, NULL) ==
>>>>> -1) {
>>>>> -			PERROR("epoll_ctl: failed to delete socket");
>>>>> -		}
>>>>> +		epoll_ctl(sock->epoll_fd, EPOLL_CTL_DEL, sock->fd, NULL);
>>>>
>>>> I think we should keep the error value test here ?
>>>
>>> No, EPOLL_CTL_DEL can only return error ENONENT, if there was no fd  
>>> with
>>> that fd number, in which cas everything anyway is fine.
>>
>> This is the kind of obvious safety net we should leave in place. It's
>> not because we expect that we'll always pass a valid FD that we won't
>> have a bug creeping in at some point, and checking the returned error
>> value is the kind of safety measure that will help us diagnose  
>> problems
>> much more quickly in the long run.
> hm. Remember, this is a tracing library. It should be quiet.
>
> What do you suggest we do with the error? the function doesn't return  
> anything.
> The only thing we can do is spew out a error message, which if we are  
> lucky may be reported, probably be ignored, and in worst case screw up a 
> program using stdout/stderr for something.
>
> Maybe we should discuss this though. What should UST do/not-do. Should  
> UST ever throw stuff out on stdout/stderr?

Aren't these text error messages already channeled through separate file
descriptors through UST-specific log files which are saved along the
recorded traces ?

>
>>
>>>
>>>>
>>>>> 	}
>>>>> 	close(sock->fd);
>>>>> 	free(sock);
>>>>> @@ -168,13 +166,11 @@ struct ustcomm_sock *
>>>>> ustcomm_init_named_socket(const char *name,
>>>>>
>>>>> 	fd = socket(PF_UNIX, SOCK_STREAM, 0);
>>>>> 	if(fd == -1) {
>>>>> -		PERROR("socket");
>>>>> 		return NULL;
>>>>> 	}
>>>>>
>>>>> 	addr = create_sock_addr(name, &sock_addr_size);
>>>>> 	if (addr == NULL) {
>>>>> -		ERR("allocating addr, UST thread bailing");
>>>>> 		goto close_sock;
>>>>> 	}
>>>>>
>>>>> @@ -182,29 +178,24 @@ struct ustcomm_sock *
>>>>> ustcomm_init_named_socket(const char *name,
>>>>> 	if(result == 0) {
>>>>> 		/* file exists */
>>>>> 		result = unlink(name);
>>>>> -		if(result == -1) {
>>>>> -			PERROR("unlink of socket file");
>>>>> +		if (result == -1) {
>>>>> 			goto free_addr;
>>>>> 		}
>>>>> -		DBG("socket already exists; overwriting");
>>>>> 	}
>>>>>
>>>>> 	result = bind(fd, (struct sockaddr *)addr, sock_addr_size);
>>>>> 	if(result == -1) {
>>>>> -		PERROR("bind");
>>>>> 		goto free_addr;
>>>>> 	}
>>>>>
>>>>> 	result = listen(fd, 1);
>>>>> 	if(result == -1) {
>>>>> -		PERROR("listen");
>>>>> 		goto free_addr;
>>>>> 	}
>>>>>
>>>>> 	sock = ustcomm_init_sock(fd, epoll_fd,
>>>>> 				 NULL);
>>>>> 	if (!sock) {
>>>>> -		ERR("failed to create ustcomm_sock");
>>>>> 		goto free_addr;
>>>>> 	}
>>>>>
>>>>> @@ -236,39 +227,31 @@ void ustcomm_del_named_sock(struct
>>>>> ustcomm_sock *sock,
>>>>> 		/* Get the socket name */
>>>>> 		alloc_size = sizeof(dummy);
>>>>> 		if (getsockname(fd, &dummy, (socklen_t *)&alloc_size) < 0) {
>>>>> -			PERROR("getsockname failed");
>>>>> 			goto del_sock;
>>>>> 		}
>>>>>
>>>>> 		sockaddr = zmalloc(alloc_size);
>>>>> 		if (!sockaddr) {
>>>>> -			ERR("failed to allocate sockaddr");
>>>>> 			goto del_sock;
>>>>> 		}
>>>>>
>>>>> 		if (getsockname(fd, sockaddr, (socklen_t *)&alloc_size) < 0) {
>>>>> -			PERROR("getsockname failed");
>>>>> 			goto free_sockaddr;
>>>>> 		}
>>>>>
>>>>> 		/* Destroy socket */
>>>>> 		result = stat(sockaddr->sun_path, &st);
>>>>> 		if(result < 0) {
>>>>> -			PERROR("stat (%s)", sockaddr->sun_path);
>>>>> 			goto free_sockaddr;
>>>>> 		}
>>>>>
>>>>> 		/* Paranoid check before deleting. */
>>>>> 		result = S_ISSOCK(st.st_mode);
>>>>> 		if(!result) {
>>>>> -			ERR("The socket we are about to delete is not a socket.");
>>>>> 			goto free_sockaddr;
>>>>> 		}
>>>>>
>>>>> -		result = unlink(sockaddr->sun_path);
>>>>> -		if(result < 0) {
>>>>> -			PERROR("unlink");
>>>>> -		}
>>>>> +		unlink(sockaddr->sun_path);
>>>>
>>>> Should we do something with the error here ? Why is it not treated  
>>>> (if
>>>> there is a reason, we should add a comment)
>>>
>>> hm, could add a comment but not much point. This is just cleanup  
>>> after
>>> all, if it fails we can't do anything about it and it won't affect
>>> anything.
>>
>> I recommend this to ensure that all return values are either
>>
>> - treated, or
>> - documented.
>>
>> looking at unlink(2), there are many reasons why unlink can fail. If  
>> it
>> fails due to incorrect access rights (for instance), we probably want 
>> to
>> propagate the error up to the user, no ?
>
> For what? Nothing the user can/should do anyway.

Reporting when the developer/distribution screw things up, mostly. :)
I've got used to expect all the unexpected ways things can go wrong with
projects deployed in a wide range of environments by yet unknown people.

Reporting errors should not be about reporting expected error cases, but
covering all the paths.

Thanks,

Mathieu

>
> /Nils
>>
>> Thanks,
>>
>> Mathieu
>>
>>>
>>> /Nils
>>>
>>>>
>>>> Thanks,
>>>>
>>>> Mathieu
>>>>
>>>>> 	}
>>>>>
>>>>> free_sockaddr:
>>>>> @@ -297,7 +280,6 @@ int ustcomm_recv_alloc(int sock,
>>>>> 		} else if (errno == EINTR) {
>>>>> 			return -1;
>>>>> 		} else if (result < 0) {
>>>>> -			PERROR("recv");
>>>>> 			return -1;
>>>>> 		}
>>>>> 		return 0;
>>>>> @@ -326,7 +308,6 @@ int ustcomm_recv_alloc(int sock,
>>>>> 	result = recvmsg(sock, &msg, MSG_WAITALL);
>>>>> 	if (result < 0) {
>>>>> 		free(*data);
>>>>> -		PERROR("recvmsg failed");
>>>>> 	}
>>>>>
>>>>> 	return result;
>>>>> @@ -355,7 +336,6 @@ int ustcomm_recv_fd(int sock,
>>>>> 		} else if (errno == EINTR) {
>>>>> 			return -1;
>>>>> 		} else if (result < 0) {
>>>>> -			PERROR("recv");
>>>>> 			return -1;
>>>>> 		}
>>>>> 		return 0;
>>>>> @@ -372,7 +352,6 @@ int ustcomm_recv_fd(int sock,
>>>>> 	if (peek_header.size && data) {
>>>>> 		if (peek_header.size < 0 ||
>>>>> 		    peek_header.size > USTCOMM_DATA_SIZE) {
>>>>> -			ERR("big peek header! %ld", peek_header.size);
>>>>> 			return 0;
>>>>> 		}
>>>>>
>>>>> @@ -389,9 +368,6 @@ int ustcomm_recv_fd(int sock,
>>>>>
>>>>> 	result = recvmsg(sock, &msg, MSG_WAITALL);
>>>>> 	if (result <= 0) {
>>>>> -		if (result < 0) {
>>>>> -			PERROR("recvmsg failed");
>>>>> -		}
>>>>> 		return result;
>>>>> 	}
>>>>>
>>>>> @@ -407,9 +383,6 @@ int ustcomm_recv_fd(int sock,
>>>>> 			}
>>>>> 			cmsg = CMSG_NXTHDR(&msg, cmsg);
>>>>> 		}
>>>>> -		if (!result) {
>>>>> -			ERR("Failed to receive file descriptor\n");
>>>>> -		}
>>>>> 	}
>>>>>
>>>>> 	return 1;
>>>>> @@ -430,7 +403,6 @@ int ustcomm_send_fd(int sock,
>>>>> {
>>>>> 	struct iovec iov[2];
>>>>> 	struct msghdr msg;
>>>>> -	int result;
>>>>> 	struct cmsghdr *cmsg;
>>>>> 	char buf[CMSG_SPACE(sizeof(int))];
>>>>>
>>>>> @@ -461,11 +433,7 @@ int ustcomm_send_fd(int sock,
>>>>> 		msg.msg_controllen = cmsg->cmsg_len;
>>>>> 	}
>>>>>
>>>>> -	result = sendmsg(sock, &msg, MSG_NOSIGNAL);
>>>>> -	if (result < 0 && errno != EPIPE) {
>>>>> -		PERROR("sendmsg failed");
>>>>> -	}
>>>>> -	return result;
>>>>> +	return sendmsg(sock, &msg, MSG_NOSIGNAL);
>>>>> }
>>>>>
>>>>> int ustcomm_send(int sock,
>>>>> @@ -504,19 +472,16 @@ int ustcomm_connect_path(const char *name,  
>>>>> int
>>>>> *connection_fd)
>>>>>
>>>>> 	fd = socket(PF_UNIX, SOCK_STREAM, 0);
>>>>> 	if(fd == -1) {
>>>>> -		PERROR("socket");
>>>>> 		return -1;
>>>>> 	}
>>>>>
>>>>> 	addr = create_sock_addr(name, &sock_addr_size);
>>>>> 	if (addr == NULL) {
>>>>> -		ERR("allocating addr failed");
>>>>> 		goto close_sock;
>>>>> 	}
>>>>>
>>>>> 	result = connect(fd, (struct sockaddr *)addr, sock_addr_size);
>>>>> 	if(result == -1) {
>>>>> -		PERROR("connect (path=%s)", name);
>>>>> 		goto free_sock_addr;
>>>>> 	}
>>>>>
>>>>> @@ -543,7 +508,6 @@ char *ustcomm_user_sock_dir(void)
>>>>> 	result = asprintf(&sock_dir, "%s%s", USER_SOCK_DIR,
>>>>> 			  cuserid(NULL));
>>>>> 	if (result < 0) {
>>>>> -		ERR("string overflow allocating directory name");
>>>>> 		return NULL;
>>>>> 	}
>>>>>
>>>>> @@ -569,14 +533,12 @@ static int connect_app_non_root(pid_t pid,  
>>>>> int
>>>>> *app_fd)
>>>>>
>>>>> 	result = asprintf(&sock_name, "%s/%d", dir_name, pid);
>>>>> 	if (result < 0) {
>>>>> -		ERR("failed to allocate socket name");
>>>>> 		retval = -1;
>>>>> 		goto free_dir_name;
>>>>> 	}
>>>>>
>>>>> 	result = ustcomm_connect_path(sock_name, app_fd);
>>>>> 	if (result < 0) {
>>>>> -		ERR("failed to connect to app");
>>>>> 		retval = -1;
>>>>> 		goto free_sock_name;
>>>>> 	}
>>>>> @@ -657,14 +619,12 @@ int ensure_dir_exists(const char *dir, mode_t
>>>>> mode)
>>>>>
>>>>> 		result = mkdir_p(dir, mode);
>>>>> 		if(result != 0) {
>>>>> -			ERR("executing in recursive creation of directory %s", dir);
>>>>> 			return -1;
>>>>> 		}
>>>>> 	} else {
>>>>> 		if (st.st_mode != mode) {
>>>>> 			result = chmod(dir, mode);
>>>>> 			if (result < 0) {
>>>>> -				ERR("couldn't set directory mode on %s", dir);
>>>>> 				return -1;
>>>>> 			}
>>>>> 		}
>>>>> diff --git a/libustctl/libustctl.c b/libustctl/libustctl.c
>>>>> index 1911434..b5d6a02 100644
>>>>> --- a/libustctl/libustctl.c
>>>>> +++ b/libustctl/libustctl.c
>>>>> @@ -17,6 +17,7 @@
>>>>> */
>>>>>
>>>>> #define _GNU_SOURCE
>>>>> +#include <errno.h>
>>>>> #include <stdio.h>
>>>>> #include <unistd.h>
>>>>> #include <getopt.h>
>>>>> @@ -26,8 +27,8 @@
>>>>> #include <dirent.h>
>>>>>
>>>>> #include "ustcomm.h"
>>>>> -#include "ust/ustctl.h"
>>>>> -#include "usterr.h"
>>>>> +#include <ust/ustctl.h>
>>>>> +#include <ust/core.h>
>>>>>
>>>>> static int do_cmd(int sock,
>>>>> 		  const struct ustcomm_header *req_header,
>>>>> @@ -57,7 +58,6 @@ static int do_cmd(int sock,
>>>>> 			}
>>>>> 		}
>>>>> 	} else {
>>>>> -		ERR("ustcomm req failed");
>>>>> 		if (result == 0) {
>>>>> 			saved_errno = ENOTCONN;
>>>>> 		} else {
>>>>> @@ -80,7 +80,6 @@ int ustctl_connect_pid(pid_t pid)
>>>>> 	int sock;
>>>>>
>>>>> 	if (ustcomm_connect_app(pid, &sock)) {
>>>>> -		ERR("could not connect to PID %u", (unsigned int) pid);
>>>>> 		errno = ENOTCONN;
>>>>> 		return -1;
>>>>> 	}
>>>>> @@ -580,20 +579,17 @@ int ustctl_get_cmsf(int sock, struct
>>>>> marker_status **cmsf)
>>>>>
>>>>> 	result = ustcomm_send(sock, &req_header, NULL);
>>>>> 	if (result <= 0) {
>>>>> -		PERROR("error while requesting markers list");
>>>>> 		return -1;
>>>>> 	}
>>>>>
>>>>> 	result = ustcomm_recv_alloc(sock, &res_header, &big_str);
>>>>> 	if (result <= 0) {
>>>>> -		ERR("error while receiving markers list");
>>>>> 		return -1;
>>>>> 	}
>>>>>
>>>>> 	tmp_cmsf = (struct marker_status *) zmalloc(sizeof(struct
>>>>> marker_status) *
>>>>> 						    (ustctl_count_nl(big_str) + 1));
>>>>> 	if (tmp_cmsf == NULL) {
>>>>> -		ERR("Failed to allocate CMSF array");
>>>>> 		return -1;
>>>>> 	}
>>>>>
>>>>> @@ -672,13 +668,11 @@ int ustctl_get_tes(int sock, struct
>>>>> trace_event_status **tes)
>>>>>
>>>>> 	result = ustcomm_send(sock, &req_header, NULL);
>>>>> 	if (result != 1) {
>>>>> -		ERR("error while requesting trace_event list");
>>>>> 		return -1;
>>>>> 	}
>>>>>
>>>>> 	result = ustcomm_recv_alloc(sock, &res_header, &big_str);
>>>>> 	if (result != 1) {
>>>>> -		ERR("error while receiving markers list");
>>>>> 		return -1;
>>>>> 	}
>>>>>
>>>>> @@ -686,7 +680,6 @@ int ustctl_get_tes(int sock, struct
>>>>> trace_event_status **tes)
>>>>> 		zmalloc(sizeof(struct trace_event_status) *
>>>>> 			(ustctl_count_nl(big_str) + 1));
>>>>> 	if (tmp_tes == NULL) {
>>>>> -		ERR("Failed to allocate TES array");
>>>>> 		return -1;
>>>>> 	}
>>>>>
>>>>> -- 
>>>>> 1.7.1
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> ltt-dev mailing list
>>>>> ltt-dev at lists.casi.polymtl.ca
>>>>> http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev
>>>>>
>>>>
>>>> -- 
>>>> Mathieu Desnoyers
>>>> Operating System Efficiency R&D Consultant
>>>> EfficiOS Inc.
>>>> http://www.efficios.com
>>>>
>>>> _______________________________________________
>>>> ltt-dev mailing list
>>>> ltt-dev at lists.casi.polymtl.ca
>>>> http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev
>>>
>>
>> -- 
>> Mathieu Desnoyers
>> Operating System Efficiency R&D Consultant
>> EfficiOS Inc.
>> http://www.efficios.com
>

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

[ltt-dev] [UST PATCH] Remove all "usterr.h" references in libusctl/ustcomm

[Nils Carlson]

On Apr 1, 2011, at 10:51 PM, Mathieu Desnoyers wrote:

> * Nils Carlson (nils.carlson at ludd.ltu.se) wrote:
>>
>> On Apr 1, 2011, at 10:24 PM, Mathieu Desnoyers wrote:
>>
>>> * Nils Carlson (nils.carlson at ludd.ltu.se) wrote:
>>>>
>>>> On Apr 1, 2011, at 5:14 PM, Mathieu Desnoyers wrote:
>>>>
>>>>> * Nils Carlson (nils.carlson at ericsson.com) wrote:
>>>>>> This patch removes all references to usterr.h and functions in
>>>>>> usterr.h from libustctl and ustcomm. This is done for three
>>>>>> reasons:
>>>>>>
>>>>>> 1. DBG, ERR, PERROR etc call in ust_safe_snprintf, I don't know
>>>>>> why, and I can't figure out any good reasons. If somebody
>>>>>> can enlighten me I would be very grateful. The only thing
>>>>>> I can figure out is that these are some sort of "signal safe"
>>>>>> printf.
>>>>>
>>>>> Yep, that's right.
>>>>>
>>>>>>
>>>>>> 2. All this stuff is quite stable and working and libraries  
>>>>>> really
>>>>>> shouldn't be printing stuff.
>>>>>
>>>>> What I disagree about the patch below is that you are removing  
>>>>> debug
>>>>> error printouts without taking care of some of the error values in
>>>>> many
>>>>> cases (just ignoring them).
>>>>>
>>>>> So we start from a dubtious behavior (debug error messages was not
>>>>> the
>>>>> proper way to handle error values; they should rather have been
>>>>> treated
>>>>> and pushed to the callers), to just ignoring the errors, which is
>>>>> kind
>>>>> of bad, you know... ;)
>>>>>
>>>>> So I would expect that all the error values you remove debug
>>>>> messages
>>>>> from should be dealt with rather than ignored.
>>>>
>>>> Hm. errno's are still there... As long as errnos aren't overwritten
>>>> they
>>>> can still be used. Can't find anywhere where we are making calls  
>>>> that
>>>> should overwrite errnos. We could try to introduce consistency in
>>>> returning negative errno's though. That would be a separate patch.
>>>>>
>>>>>>
>>>>>> 3. This allows me to compile lttng-tools without linking  
>>>>>> libustctl
>>>>>> to the ust_snprintf family of functions. This makes me happy.
>>>>>
>>>>> Well, you could just create a set of debug macros that use the
>>>>> standard
>>>>> printf, and decide at compile-time if the log output is built or  
>>>>> not
>>>>> with these pretty simple macros. This change would make more sense
>>>>> to
>>>>> me
>>>>> than removing the debugging code we have there.
>>>>>
>>>> Well, could do that... but as noted, this is pretty stable stuff  
>>>> and
>>>> we
>>>> can add debug code as needed I think.
>>>>
>>>>> More comments below,
>>>>>
>>>>>>
>>>>>> Mathieu, enlighten me as best you can... :-)
>>>>>>
>>>>>> Signed-off-by: Nils Carlson <nils.carlson at ericsson.com>
>>>>>> ---
>>>>>> libustcomm/ustcomm.c  |   52 ++++
>>>>>> +-------------------------------------------
>>>>>> libustctl/libustctl.c |   13 ++---------
>>>>>> 2 files changed, 9 insertions(+), 56 deletions(-)
>>>>>>
>>>>>> diff --git a/libustcomm/ustcomm.c b/libustcomm/ustcomm.c
>>>>>> index dcf8cd8..2a5acc7 100644
>>>>>> --- a/libustcomm/ustcomm.c
>>>>>> +++ b/libustcomm/ustcomm.c
>>>>>> @@ -29,13 +29,14 @@
>>>>>> #include <sys/epoll.h>
>>>>>> #include <sys/stat.h>
>>>>>>
>>>>>> +#include <stdarg.h>
>>>>>> #include <stdio.h>
>>>>>> #include <stdlib.h>
>>>>>> #include <string.h>
>>>>>> #include <execinfo.h>
>>>>>>
>>>>>> #include "ustcomm.h"
>>>>>> -#include "usterr.h"
>>>>>> +#include <ust/core.h>
>>>>>> #include "share.h"
>>>>>>
>>>>>> static int mkdir_p(const char *path, mode_t mode)
>>>>>> @@ -101,7 +102,6 @@ static struct sockaddr_un *
>>>>>> create_sock_addr(const char *name,
>>>>>>
>>>>>> 	addr = malloc(alloc_size);
>>>>>> 	if (addr < 0) {
>>>>>
>>>>> Hrm, the original test is bogus there ? malloc returns NULL if  
>>>>> alloc
>>>>> fails, not a negative value... ?
>>>>
>>>> True... Could fix that in a seperate patch.
>>>>>
>>>>>> -		ERR("allocating addr failed");
>>>>>> 		return NULL;
>>>>>> 	}
>>>>>>
>>>>>> @@ -149,9 +149,7 @@ void ustcomm_del_sock(struct ustcomm_sock
>>>>>> *sock,
>>>>>> int keep_in_epoll)
>>>>>> {
>>>>>> 	cds_list_del(&sock->list);
>>>>>> 	if (!keep_in_epoll) {
>>>>>> -		if (epoll_ctl(sock->epoll_fd, EPOLL_CTL_DEL, sock->fd, NULL)  
>>>>>> ==
>>>>>> -1) {
>>>>>> -			PERROR("epoll_ctl: failed to delete socket");
>>>>>> -		}
>>>>>> +		epoll_ctl(sock->epoll_fd, EPOLL_CTL_DEL, sock->fd, NULL);
>>>>>
>>>>> I think we should keep the error value test here ?
>>>>
>>>> No, EPOLL_CTL_DEL can only return error ENONENT, if there was no fd
>>>> with
>>>> that fd number, in which cas everything anyway is fine.
>>>
>>> This is the kind of obvious safety net we should leave in place.  
>>> It's
>>> not because we expect that we'll always pass a valid FD that we  
>>> won't
>>> have a bug creeping in at some point, and checking the returned  
>>> error
>>> value is the kind of safety measure that will help us diagnose
>>> problems
>>> much more quickly in the long run.
>> hm. Remember, this is a tracing library. It should be quiet.
>>
>> What do you suggest we do with the error? the function doesn't return
>> anything.
>> The only thing we can do is spew out a error message, which if we are
>> lucky may be reported, probably be ignored, and in worst case screw  
>> up a
>> program using stdout/stderr for something.
>>
>> Maybe we should discuss this though. What should UST do/not-do.  
>> Should
>> UST ever throw stuff out on stdout/stderr?
>
> Aren't these text error messages already channeled through separate  
> file
> descriptors through UST-specific log files which are saved along the
> recorded traces ?
>
Nope, go straight to stderr. We could discuss logfiles though, but  
then they should be configured by an environment variable, ie. not on  
by default.

>>
>>>
>>>>
>>>>>
>>>>>> 	}
>>>>>> 	close(sock->fd);
>>>>>> 	free(sock);
>>>>>> @@ -168,13 +166,11 @@ struct ustcomm_sock *
>>>>>> ustcomm_init_named_socket(const char *name,
>>>>>>
>>>>>> 	fd = socket(PF_UNIX, SOCK_STREAM, 0);
>>>>>> 	if(fd == -1) {
>>>>>> -		PERROR("socket");
>>>>>> 		return NULL;
>>>>>> 	}
>>>>>>
>>>>>> 	addr = create_sock_addr(name, &sock_addr_size);
>>>>>> 	if (addr == NULL) {
>>>>>> -		ERR("allocating addr, UST thread bailing");
>>>>>> 		goto close_sock;
>>>>>> 	}
>>>>>>
>>>>>> @@ -182,29 +178,24 @@ struct ustcomm_sock *
>>>>>> ustcomm_init_named_socket(const char *name,
>>>>>> 	if(result == 0) {
>>>>>> 		/* file exists */
>>>>>> 		result = unlink(name);
>>>>>> -		if(result == -1) {
>>>>>> -			PERROR("unlink of socket file");
>>>>>> +		if (result == -1) {
>>>>>> 			goto free_addr;
>>>>>> 		}
>>>>>> -		DBG("socket already exists; overwriting");
>>>>>> 	}
>>>>>>
>>>>>> 	result = bind(fd, (struct sockaddr *)addr, sock_addr_size);
>>>>>> 	if(result == -1) {
>>>>>> -		PERROR("bind");
>>>>>> 		goto free_addr;
>>>>>> 	}
>>>>>>
>>>>>> 	result = listen(fd, 1);
>>>>>> 	if(result == -1) {
>>>>>> -		PERROR("listen");
>>>>>> 		goto free_addr;
>>>>>> 	}
>>>>>>
>>>>>> 	sock = ustcomm_init_sock(fd, epoll_fd,
>>>>>> 				 NULL);
>>>>>> 	if (!sock) {
>>>>>> -		ERR("failed to create ustcomm_sock");
>>>>>> 		goto free_addr;
>>>>>> 	}
>>>>>>
>>>>>> @@ -236,39 +227,31 @@ void ustcomm_del_named_sock(struct
>>>>>> ustcomm_sock *sock,
>>>>>> 		/* Get the socket name */
>>>>>> 		alloc_size = sizeof(dummy);
>>>>>> 		if (getsockname(fd, &dummy, (socklen_t *)&alloc_size) < 0) {
>>>>>> -			PERROR("getsockname failed");
>>>>>> 			goto del_sock;
>>>>>> 		}
>>>>>>
>>>>>> 		sockaddr = zmalloc(alloc_size);
>>>>>> 		if (!sockaddr) {
>>>>>> -			ERR("failed to allocate sockaddr");
>>>>>> 			goto del_sock;
>>>>>> 		}
>>>>>>
>>>>>> 		if (getsockname(fd, sockaddr, (socklen_t *)&alloc_size) < 0) {
>>>>>> -			PERROR("getsockname failed");
>>>>>> 			goto free_sockaddr;
>>>>>> 		}
>>>>>>
>>>>>> 		/* Destroy socket */
>>>>>> 		result = stat(sockaddr->sun_path, &st);
>>>>>> 		if(result < 0) {
>>>>>> -			PERROR("stat (%s)", sockaddr->sun_path);
>>>>>> 			goto free_sockaddr;
>>>>>> 		}
>>>>>>
>>>>>> 		/* Paranoid check before deleting. */
>>>>>> 		result = S_ISSOCK(st.st_mode);
>>>>>> 		if(!result) {
>>>>>> -			ERR("The socket we are about to delete is not a socket.");
>>>>>> 			goto free_sockaddr;
>>>>>> 		}
>>>>>>
>>>>>> -		result = unlink(sockaddr->sun_path);
>>>>>> -		if(result < 0) {
>>>>>> -			PERROR("unlink");
>>>>>> -		}
>>>>>> +		unlink(sockaddr->sun_path);
>>>>>
>>>>> Should we do something with the error here ? Why is it not treated
>>>>> (if
>>>>> there is a reason, we should add a comment)
>>>>
>>>> hm, could add a comment but not much point. This is just cleanup
>>>> after
>>>> all, if it fails we can't do anything about it and it won't affect
>>>> anything.
>>>
>>> I recommend this to ensure that all return values are either
>>>
>>> - treated, or
>>> - documented.
>>>
>>> looking at unlink(2), there are many reasons why unlink can fail. If
>>> it
>>> fails due to incorrect access rights (for instance), we probably  
>>> want
>>> to
>>> propagate the error up to the user, no ?
>>
>> For what? Nothing the user can/should do anyway.
>
> Reporting when the developer/distribution screw things up, mostly. :)
> I've got used to expect all the unexpected ways things can go wrong  
> with
> projects deployed in a wide range of environments by yet unknown  
> people.
>
> Reporting errors should not be about reporting expected error cases,  
> but
> covering all the paths.
>

Sounds good in theory, but again, what do we want to send out? this is  
a void function.

But the logfile turned on by an environment variable may not be a bad  
idea. This way users could turn it on to check for strange behaviour.

Why do we want the signal safe printf by the way? When are we using  
this stuff in signal handlers?

/Nils

> Thanks,
>
> Mathieu
>
>>
>> /Nils
>>>
>>> Thanks,
>>>
>>> Mathieu
>>>
>>>>
>>>> /Nils
>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Mathieu
>>>>>
>>>>>> 	}
>>>>>>
>>>>>> free_sockaddr:
>>>>>> @@ -297,7 +280,6 @@ int ustcomm_recv_alloc(int sock,
>>>>>> 		} else if (errno == EINTR) {
>>>>>> 			return -1;
>>>>>> 		} else if (result < 0) {
>>>>>> -			PERROR("recv");
>>>>>> 			return -1;
>>>>>> 		}
>>>>>> 		return 0;
>>>>>> @@ -326,7 +308,6 @@ int ustcomm_recv_alloc(int sock,
>>>>>> 	result = recvmsg(sock, &msg, MSG_WAITALL);
>>>>>> 	if (result < 0) {
>>>>>> 		free(*data);
>>>>>> -		PERROR("recvmsg failed");
>>>>>> 	}
>>>>>>
>>>>>> 	return result;
>>>>>> @@ -355,7 +336,6 @@ int ustcomm_recv_fd(int sock,
>>>>>> 		} else if (errno == EINTR) {
>>>>>> 			return -1;
>>>>>> 		} else if (result < 0) {
>>>>>> -			PERROR("recv");
>>>>>> 			return -1;
>>>>>> 		}
>>>>>> 		return 0;
>>>>>> @@ -372,7 +352,6 @@ int ustcomm_recv_fd(int sock,
>>>>>> 	if (peek_header.size && data) {
>>>>>> 		if (peek_header.size < 0 ||
>>>>>> 		    peek_header.size > USTCOMM_DATA_SIZE) {
>>>>>> -			ERR("big peek header! %ld", peek_header.size);
>>>>>> 			return 0;
>>>>>> 		}
>>>>>>
>>>>>> @@ -389,9 +368,6 @@ int ustcomm_recv_fd(int sock,
>>>>>>
>>>>>> 	result = recvmsg(sock, &msg, MSG_WAITALL);
>>>>>> 	if (result <= 0) {
>>>>>> -		if (result < 0) {
>>>>>> -			PERROR("recvmsg failed");
>>>>>> -		}
>>>>>> 		return result;
>>>>>> 	}
>>>>>>
>>>>>> @@ -407,9 +383,6 @@ int ustcomm_recv_fd(int sock,
>>>>>> 			}
>>>>>> 			cmsg = CMSG_NXTHDR(&msg, cmsg);
>>>>>> 		}
>>>>>> -		if (!result) {
>>>>>> -			ERR("Failed to receive file descriptor\n");
>>>>>> -		}
>>>>>> 	}
>>>>>>
>>>>>> 	return 1;
>>>>>> @@ -430,7 +403,6 @@ int ustcomm_send_fd(int sock,
>>>>>> {
>>>>>> 	struct iovec iov[2];
>>>>>> 	struct msghdr msg;
>>>>>> -	int result;
>>>>>> 	struct cmsghdr *cmsg;
>>>>>> 	char buf[CMSG_SPACE(sizeof(int))];
>>>>>>
>>>>>> @@ -461,11 +433,7 @@ int ustcomm_send_fd(int sock,
>>>>>> 		msg.msg_controllen = cmsg->cmsg_len;
>>>>>> 	}
>>>>>>
>>>>>> -	result = sendmsg(sock, &msg, MSG_NOSIGNAL);
>>>>>> -	if (result < 0 && errno != EPIPE) {
>>>>>> -		PERROR("sendmsg failed");
>>>>>> -	}
>>>>>> -	return result;
>>>>>> +	return sendmsg(sock, &msg, MSG_NOSIGNAL);
>>>>>> }
>>>>>>
>>>>>> int ustcomm_send(int sock,
>>>>>> @@ -504,19 +472,16 @@ int ustcomm_connect_path(const char *name,
>>>>>> int
>>>>>> *connection_fd)
>>>>>>
>>>>>> 	fd = socket(PF_UNIX, SOCK_STREAM, 0);
>>>>>> 	if(fd == -1) {
>>>>>> -		PERROR("socket");
>>>>>> 		return -1;
>>>>>> 	}
>>>>>>
>>>>>> 	addr = create_sock_addr(name, &sock_addr_size);
>>>>>> 	if (addr == NULL) {
>>>>>> -		ERR("allocating addr failed");
>>>>>> 		goto close_sock;
>>>>>> 	}
>>>>>>
>>>>>> 	result = connect(fd, (struct sockaddr *)addr, sock_addr_size);
>>>>>> 	if(result == -1) {
>>>>>> -		PERROR("connect (path=%s)", name);
>>>>>> 		goto free_sock_addr;
>>>>>> 	}
>>>>>>
>>>>>> @@ -543,7 +508,6 @@ char *ustcomm_user_sock_dir(void)
>>>>>> 	result = asprintf(&sock_dir, "%s%s", USER_SOCK_DIR,
>>>>>> 			  cuserid(NULL));
>>>>>> 	if (result < 0) {
>>>>>> -		ERR("string overflow allocating directory name");
>>>>>> 		return NULL;
>>>>>> 	}
>>>>>>
>>>>>> @@ -569,14 +533,12 @@ static int connect_app_non_root(pid_t pid,
>>>>>> int
>>>>>> *app_fd)
>>>>>>
>>>>>> 	result = asprintf(&sock_name, "%s/%d", dir_name, pid);
>>>>>> 	if (result < 0) {
>>>>>> -		ERR("failed to allocate socket name");
>>>>>> 		retval = -1;
>>>>>> 		goto free_dir_name;
>>>>>> 	}
>>>>>>
>>>>>> 	result = ustcomm_connect_path(sock_name, app_fd);
>>>>>> 	if (result < 0) {
>>>>>> -		ERR("failed to connect to app");
>>>>>> 		retval = -1;
>>>>>> 		goto free_sock_name;
>>>>>> 	}
>>>>>> @@ -657,14 +619,12 @@ int ensure_dir_exists(const char *dir,  
>>>>>> mode_t
>>>>>> mode)
>>>>>>
>>>>>> 		result = mkdir_p(dir, mode);
>>>>>> 		if(result != 0) {
>>>>>> -			ERR("executing in recursive creation of directory %s", dir);
>>>>>> 			return -1;
>>>>>> 		}
>>>>>> 	} else {
>>>>>> 		if (st.st_mode != mode) {
>>>>>> 			result = chmod(dir, mode);
>>>>>> 			if (result < 0) {
>>>>>> -				ERR("couldn't set directory mode on %s", dir);
>>>>>> 				return -1;
>>>>>> 			}
>>>>>> 		}
>>>>>> diff --git a/libustctl/libustctl.c b/libustctl/libustctl.c
>>>>>> index 1911434..b5d6a02 100644
>>>>>> --- a/libustctl/libustctl.c
>>>>>> +++ b/libustctl/libustctl.c
>>>>>> @@ -17,6 +17,7 @@
>>>>>> */
>>>>>>
>>>>>> #define _GNU_SOURCE
>>>>>> +#include <errno.h>
>>>>>> #include <stdio.h>
>>>>>> #include <unistd.h>
>>>>>> #include <getopt.h>
>>>>>> @@ -26,8 +27,8 @@
>>>>>> #include <dirent.h>
>>>>>>
>>>>>> #include "ustcomm.h"
>>>>>> -#include "ust/ustctl.h"
>>>>>> -#include "usterr.h"
>>>>>> +#include <ust/ustctl.h>
>>>>>> +#include <ust/core.h>
>>>>>>
>>>>>> static int do_cmd(int sock,
>>>>>> 		  const struct ustcomm_header *req_header,
>>>>>> @@ -57,7 +58,6 @@ static int do_cmd(int sock,
>>>>>> 			}
>>>>>> 		}
>>>>>> 	} else {
>>>>>> -		ERR("ustcomm req failed");
>>>>>> 		if (result == 0) {
>>>>>> 			saved_errno = ENOTCONN;
>>>>>> 		} else {
>>>>>> @@ -80,7 +80,6 @@ int ustctl_connect_pid(pid_t pid)
>>>>>> 	int sock;
>>>>>>
>>>>>> 	if (ustcomm_connect_app(pid, &sock)) {
>>>>>> -		ERR("could not connect to PID %u", (unsigned int) pid);
>>>>>> 		errno = ENOTCONN;
>>>>>> 		return -1;
>>>>>> 	}
>>>>>> @@ -580,20 +579,17 @@ int ustctl_get_cmsf(int sock, struct
>>>>>> marker_status **cmsf)
>>>>>>
>>>>>> 	result = ustcomm_send(sock, &req_header, NULL);
>>>>>> 	if (result <= 0) {
>>>>>> -		PERROR("error while requesting markers list");
>>>>>> 		return -1;
>>>>>> 	}
>>>>>>
>>>>>> 	result = ustcomm_recv_alloc(sock, &res_header, &big_str);
>>>>>> 	if (result <= 0) {
>>>>>> -		ERR("error while receiving markers list");
>>>>>> 		return -1;
>>>>>> 	}
>>>>>>
>>>>>> 	tmp_cmsf = (struct marker_status *) zmalloc(sizeof(struct
>>>>>> marker_status) *
>>>>>> 						    (ustctl_count_nl(big_str) + 1));
>>>>>> 	if (tmp_cmsf == NULL) {
>>>>>> -		ERR("Failed to allocate CMSF array");
>>>>>> 		return -1;
>>>>>> 	}
>>>>>>
>>>>>> @@ -672,13 +668,11 @@ int ustctl_get_tes(int sock, struct
>>>>>> trace_event_status **tes)
>>>>>>
>>>>>> 	result = ustcomm_send(sock, &req_header, NULL);
>>>>>> 	if (result != 1) {
>>>>>> -		ERR("error while requesting trace_event list");
>>>>>> 		return -1;
>>>>>> 	}
>>>>>>
>>>>>> 	result = ustcomm_recv_alloc(sock, &res_header, &big_str);
>>>>>> 	if (result != 1) {
>>>>>> -		ERR("error while receiving markers list");
>>>>>> 		return -1;
>>>>>> 	}
>>>>>>
>>>>>> @@ -686,7 +680,6 @@ int ustctl_get_tes(int sock, struct
>>>>>> trace_event_status **tes)
>>>>>> 		zmalloc(sizeof(struct trace_event_status) *
>>>>>> 			(ustctl_count_nl(big_str) + 1));
>>>>>> 	if (tmp_tes == NULL) {
>>>>>> -		ERR("Failed to allocate TES array");
>>>>>> 		return -1;
>>>>>> 	}
>>>>>>
>>>>>> -- 
>>>>>> 1.7.1
>>>>>>
>>>>>>
>>>>>> _______________________________________________
>>>>>> ltt-dev mailing list
>>>>>> ltt-dev at lists.casi.polymtl.ca
>>>>>> http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev
>>>>>>
>>>>>
>>>>> -- 
>>>>> Mathieu Desnoyers
>>>>> Operating System Efficiency R&D Consultant
>>>>> EfficiOS Inc.
>>>>> http://www.efficios.com
>>>>>
>>>>> _______________________________________________
>>>>> ltt-dev mailing list
>>>>> ltt-dev at lists.casi.polymtl.ca
>>>>> http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev
>>>>
>>>
>>> -- 
>>> Mathieu Desnoyers
>>> Operating System Efficiency R&D Consultant
>>> EfficiOS Inc.
>>> http://www.efficios.com
>>
>
> -- 
> Mathieu Desnoyers
> Operating System Efficiency R&D Consultant
> EfficiOS Inc.
> http://www.efficios.com

[ltt-dev] [UST PATCH] Remove all "usterr.h" references in libusctl/ustcomm

[Mathieu Desnoyers]

* Nils Carlson (nils.carlson at ludd.ltu.se) wrote:
>
> On Apr 1, 2011, at 10:51 PM, Mathieu Desnoyers wrote:
>
>> * Nils Carlson (nils.carlson at ludd.ltu.se) wrote:
>>>
>>> On Apr 1, 2011, at 10:24 PM, Mathieu Desnoyers wrote:
>>>
>>>> * Nils Carlson (nils.carlson at ludd.ltu.se) wrote:
>>>>>
>>>>> On Apr 1, 2011, at 5:14 PM, Mathieu Desnoyers wrote:
>>>>>
>>>>>> * Nils Carlson (nils.carlson at ericsson.com) wrote:
>>>>>>> This patch removes all references to usterr.h and functions in
>>>>>>> usterr.h from libustctl and ustcomm. This is done for three
>>>>>>> reasons:
>>>>>>>
>>>>>>> 1. DBG, ERR, PERROR etc call in ust_safe_snprintf, I don't know
>>>>>>> why, and I can't figure out any good reasons. If somebody
>>>>>>> can enlighten me I would be very grateful. The only thing
>>>>>>> I can figure out is that these are some sort of "signal safe"
>>>>>>> printf.
>>>>>>
>>>>>> Yep, that's right.
>>>>>>
>>>>>>>
>>>>>>> 2. All this stuff is quite stable and working and libraries  
>>>>>>> really
>>>>>>> shouldn't be printing stuff.
>>>>>>
>>>>>> What I disagree about the patch below is that you are removing  
>>>>>> debug
>>>>>> error printouts without taking care of some of the error values in
>>>>>> many
>>>>>> cases (just ignoring them).
>>>>>>
>>>>>> So we start from a dubtious behavior (debug error messages was not
>>>>>> the
>>>>>> proper way to handle error values; they should rather have been
>>>>>> treated
>>>>>> and pushed to the callers), to just ignoring the errors, which is
>>>>>> kind
>>>>>> of bad, you know... ;)
>>>>>>
>>>>>> So I would expect that all the error values you remove debug
>>>>>> messages
>>>>>> from should be dealt with rather than ignored.
>>>>>
>>>>> Hm. errno's are still there... As long as errnos aren't overwritten
>>>>> they
>>>>> can still be used. Can't find anywhere where we are making calls  
>>>>> that
>>>>> should overwrite errnos. We could try to introduce consistency in
>>>>> returning negative errno's though. That would be a separate patch.
>>>>>>
>>>>>>>
>>>>>>> 3. This allows me to compile lttng-tools without linking  
>>>>>>> libustctl
>>>>>>> to the ust_snprintf family of functions. This makes me happy.
>>>>>>
>>>>>> Well, you could just create a set of debug macros that use the
>>>>>> standard
>>>>>> printf, and decide at compile-time if the log output is built 
>>>>>> or not
>>>>>> with these pretty simple macros. This change would make more sense
>>>>>> to
>>>>>> me
>>>>>> than removing the debugging code we have there.
>>>>>>
>>>>> Well, could do that... but as noted, this is pretty stable stuff  
>>>>> and
>>>>> we
>>>>> can add debug code as needed I think.
>>>>>
>>>>>> More comments below,
>>>>>>
>>>>>>>
>>>>>>> Mathieu, enlighten me as best you can... :-)
>>>>>>>
>>>>>>> Signed-off-by: Nils Carlson <nils.carlson at ericsson.com>
>>>>>>> ---
>>>>>>> libustcomm/ustcomm.c  |   52 ++++
>>>>>>> +-------------------------------------------
>>>>>>> libustctl/libustctl.c |   13 ++---------
>>>>>>> 2 files changed, 9 insertions(+), 56 deletions(-)
>>>>>>>
>>>>>>> diff --git a/libustcomm/ustcomm.c b/libustcomm/ustcomm.c
>>>>>>> index dcf8cd8..2a5acc7 100644
>>>>>>> --- a/libustcomm/ustcomm.c
>>>>>>> +++ b/libustcomm/ustcomm.c
>>>>>>> @@ -29,13 +29,14 @@
>>>>>>> #include <sys/epoll.h>
>>>>>>> #include <sys/stat.h>
>>>>>>>
>>>>>>> +#include <stdarg.h>
>>>>>>> #include <stdio.h>
>>>>>>> #include <stdlib.h>
>>>>>>> #include <string.h>
>>>>>>> #include <execinfo.h>
>>>>>>>
>>>>>>> #include "ustcomm.h"
>>>>>>> -#include "usterr.h"
>>>>>>> +#include <ust/core.h>
>>>>>>> #include "share.h"
>>>>>>>
>>>>>>> static int mkdir_p(const char *path, mode_t mode)
>>>>>>> @@ -101,7 +102,6 @@ static struct sockaddr_un *
>>>>>>> create_sock_addr(const char *name,
>>>>>>>
>>>>>>> 	addr = malloc(alloc_size);
>>>>>>> 	if (addr < 0) {
>>>>>>
>>>>>> Hrm, the original test is bogus there ? malloc returns NULL if  
>>>>>> alloc
>>>>>> fails, not a negative value... ?
>>>>>
>>>>> True... Could fix that in a seperate patch.
>>>>>>
>>>>>>> -		ERR("allocating addr failed");
>>>>>>> 		return NULL;
>>>>>>> 	}
>>>>>>>
>>>>>>> @@ -149,9 +149,7 @@ void ustcomm_del_sock(struct ustcomm_sock
>>>>>>> *sock,
>>>>>>> int keep_in_epoll)
>>>>>>> {
>>>>>>> 	cds_list_del(&sock->list);
>>>>>>> 	if (!keep_in_epoll) {
>>>>>>> -		if (epoll_ctl(sock->epoll_fd, EPOLL_CTL_DEL, sock->fd, 
>>>>>>> NULL) ==
>>>>>>> -1) {
>>>>>>> -			PERROR("epoll_ctl: failed to delete socket");
>>>>>>> -		}
>>>>>>> +		epoll_ctl(sock->epoll_fd, EPOLL_CTL_DEL, sock->fd, NULL);
>>>>>>
>>>>>> I think we should keep the error value test here ?
>>>>>
>>>>> No, EPOLL_CTL_DEL can only return error ENONENT, if there was no fd
>>>>> with
>>>>> that fd number, in which cas everything anyway is fine.
>>>>
>>>> This is the kind of obvious safety net we should leave in place.  
>>>> It's
>>>> not because we expect that we'll always pass a valid FD that we  
>>>> won't
>>>> have a bug creeping in at some point, and checking the returned  
>>>> error
>>>> value is the kind of safety measure that will help us diagnose
>>>> problems
>>>> much more quickly in the long run.
>>> hm. Remember, this is a tracing library. It should be quiet.
>>>
>>> What do you suggest we do with the error? the function doesn't return
>>> anything.
>>> The only thing we can do is spew out a error message, which if we are
>>> lucky may be reported, probably be ignored, and in worst case screw  
>>> up a
>>> program using stdout/stderr for something.
>>>
>>> Maybe we should discuss this though. What should UST do/not-do.  
>>> Should
>>> UST ever throw stuff out on stdout/stderr?
>>
>> Aren't these text error messages already channeled through separate  
>> file
>> descriptors through UST-specific log files which are saved along the
>> recorded traces ?
>>
> Nope, go straight to stderr. We could discuss logfiles though, but then 
> they should be configured by an environment variable, ie. not on by 
> default.

It depends. This brings the general question of error handling in
libust. Basically, I would recommend an approach similar to what I do in
the Linux kernel, where a tracer error just disables tracing rather than
stop the kernel (or in this case, does not kill the application).

So I don't see that showing the error messages in the app stderr is very
much appropriate, because we can be tracing applications that have
stderr redirected to /dev/null, so trying to use the normal stderr
output of the application can hide problems. On the other hand, if we
send libust error output to ltt-sessiond, so it can be sent to the
"lttng" command line tool, then the user interacting with the tracing
session will have the error output of all the programs it controls in a
centralized location, which seems much more reliable.

>
>>>
>>>>
>>>>>
>>>>>>
>>>>>>> 	}
>>>>>>> 	close(sock->fd);
>>>>>>> 	free(sock);
>>>>>>> @@ -168,13 +166,11 @@ struct ustcomm_sock *
>>>>>>> ustcomm_init_named_socket(const char *name,
>>>>>>>
>>>>>>> 	fd = socket(PF_UNIX, SOCK_STREAM, 0);
>>>>>>> 	if(fd == -1) {
>>>>>>> -		PERROR("socket");
>>>>>>> 		return NULL;
>>>>>>> 	}
>>>>>>>
>>>>>>> 	addr = create_sock_addr(name, &sock_addr_size);
>>>>>>> 	if (addr == NULL) {
>>>>>>> -		ERR("allocating addr, UST thread bailing");
>>>>>>> 		goto close_sock;
>>>>>>> 	}
>>>>>>>
>>>>>>> @@ -182,29 +178,24 @@ struct ustcomm_sock *
>>>>>>> ustcomm_init_named_socket(const char *name,
>>>>>>> 	if(result == 0) {
>>>>>>> 		/* file exists */
>>>>>>> 		result = unlink(name);
>>>>>>> -		if(result == -1) {
>>>>>>> -			PERROR("unlink of socket file");
>>>>>>> +		if (result == -1) {
>>>>>>> 			goto free_addr;
>>>>>>> 		}
>>>>>>> -		DBG("socket already exists; overwriting");
>>>>>>> 	}
>>>>>>>
>>>>>>> 	result = bind(fd, (struct sockaddr *)addr, sock_addr_size);
>>>>>>> 	if(result == -1) {
>>>>>>> -		PERROR("bind");
>>>>>>> 		goto free_addr;
>>>>>>> 	}
>>>>>>>
>>>>>>> 	result = listen(fd, 1);
>>>>>>> 	if(result == -1) {
>>>>>>> -		PERROR("listen");
>>>>>>> 		goto free_addr;
>>>>>>> 	}
>>>>>>>
>>>>>>> 	sock = ustcomm_init_sock(fd, epoll_fd,
>>>>>>> 				 NULL);
>>>>>>> 	if (!sock) {
>>>>>>> -		ERR("failed to create ustcomm_sock");
>>>>>>> 		goto free_addr;
>>>>>>> 	}
>>>>>>>
>>>>>>> @@ -236,39 +227,31 @@ void ustcomm_del_named_sock(struct
>>>>>>> ustcomm_sock *sock,
>>>>>>> 		/* Get the socket name */
>>>>>>> 		alloc_size = sizeof(dummy);
>>>>>>> 		if (getsockname(fd, &dummy, (socklen_t *)&alloc_size) < 0) {
>>>>>>> -			PERROR("getsockname failed");
>>>>>>> 			goto del_sock;
>>>>>>> 		}
>>>>>>>
>>>>>>> 		sockaddr = zmalloc(alloc_size);
>>>>>>> 		if (!sockaddr) {
>>>>>>> -			ERR("failed to allocate sockaddr");
>>>>>>> 			goto del_sock;
>>>>>>> 		}
>>>>>>>
>>>>>>> 		if (getsockname(fd, sockaddr, (socklen_t *)&alloc_size) < 0) {
>>>>>>> -			PERROR("getsockname failed");
>>>>>>> 			goto free_sockaddr;
>>>>>>> 		}
>>>>>>>
>>>>>>> 		/* Destroy socket */
>>>>>>> 		result = stat(sockaddr->sun_path, &st);
>>>>>>> 		if(result < 0) {
>>>>>>> -			PERROR("stat (%s)", sockaddr->sun_path);
>>>>>>> 			goto free_sockaddr;
>>>>>>> 		}
>>>>>>>
>>>>>>> 		/* Paranoid check before deleting. */
>>>>>>> 		result = S_ISSOCK(st.st_mode);
>>>>>>> 		if(!result) {
>>>>>>> -			ERR("The socket we are about to delete is not a socket.");
>>>>>>> 			goto free_sockaddr;
>>>>>>> 		}
>>>>>>>
>>>>>>> -		result = unlink(sockaddr->sun_path);
>>>>>>> -		if(result < 0) {
>>>>>>> -			PERROR("unlink");
>>>>>>> -		}
>>>>>>> +		unlink(sockaddr->sun_path);
>>>>>>
>>>>>> Should we do something with the error here ? Why is it not treated
>>>>>> (if
>>>>>> there is a reason, we should add a comment)
>>>>>
>>>>> hm, could add a comment but not much point. This is just cleanup
>>>>> after
>>>>> all, if it fails we can't do anything about it and it won't affect
>>>>> anything.
>>>>
>>>> I recommend this to ensure that all return values are either
>>>>
>>>> - treated, or
>>>> - documented.
>>>>
>>>> looking at unlink(2), there are many reasons why unlink can fail. If
>>>> it
>>>> fails due to incorrect access rights (for instance), we probably  
>>>> want
>>>> to
>>>> propagate the error up to the user, no ?
>>>
>>> For what? Nothing the user can/should do anyway.
>>
>> Reporting when the developer/distribution screw things up, mostly. :)
>> I've got used to expect all the unexpected ways things can go wrong  
>> with
>> projects deployed in a wide range of environments by yet unknown  
>> people.
>>
>> Reporting errors should not be about reporting expected error cases,  
>> but
>> covering all the paths.
>>
>
> Sounds good in theory, but again, what do we want to send out? this is a 
> void function.

I agree that this is a teardown function, so normally returns void. But
we could at least print the error message to tell the user that
something is fishy.

>
> But the logfile turned on by an environment variable may not be a bad  
> idea. This way users could turn it on to check for strange behaviour.
>
> Why do we want the signal safe printf by the way? When are we using this 
> stuff in signal handlers?

This is used when printing error/debugging from the tracing probe site,
which can be run in signal context (if signal handlers are
instrumented).

Thanks,

Mathieu

>
> /Nils
>
>> Thanks,
>>
>> Mathieu
>>
>>>
>>> /Nils
>>>>
>>>> Thanks,
>>>>
>>>> Mathieu
>>>>
>>>>>
>>>>> /Nils
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> Mathieu
>>>>>>
>>>>>>> 	}
>>>>>>>
>>>>>>> free_sockaddr:
>>>>>>> @@ -297,7 +280,6 @@ int ustcomm_recv_alloc(int sock,
>>>>>>> 		} else if (errno == EINTR) {
>>>>>>> 			return -1;
>>>>>>> 		} else if (result < 0) {
>>>>>>> -			PERROR("recv");
>>>>>>> 			return -1;
>>>>>>> 		}
>>>>>>> 		return 0;
>>>>>>> @@ -326,7 +308,6 @@ int ustcomm_recv_alloc(int sock,
>>>>>>> 	result = recvmsg(sock, &msg, MSG_WAITALL);
>>>>>>> 	if (result < 0) {
>>>>>>> 		free(*data);
>>>>>>> -		PERROR("recvmsg failed");
>>>>>>> 	}
>>>>>>>
>>>>>>> 	return result;
>>>>>>> @@ -355,7 +336,6 @@ int ustcomm_recv_fd(int sock,
>>>>>>> 		} else if (errno == EINTR) {
>>>>>>> 			return -1;
>>>>>>> 		} else if (result < 0) {
>>>>>>> -			PERROR("recv");
>>>>>>> 			return -1;
>>>>>>> 		}
>>>>>>> 		return 0;
>>>>>>> @@ -372,7 +352,6 @@ int ustcomm_recv_fd(int sock,
>>>>>>> 	if (peek_header.size && data) {
>>>>>>> 		if (peek_header.size < 0 ||
>>>>>>> 		    peek_header.size > USTCOMM_DATA_SIZE) {
>>>>>>> -			ERR("big peek header! %ld", peek_header.size);
>>>>>>> 			return 0;
>>>>>>> 		}
>>>>>>>
>>>>>>> @@ -389,9 +368,6 @@ int ustcomm_recv_fd(int sock,
>>>>>>>
>>>>>>> 	result = recvmsg(sock, &msg, MSG_WAITALL);
>>>>>>> 	if (result <= 0) {
>>>>>>> -		if (result < 0) {
>>>>>>> -			PERROR("recvmsg failed");
>>>>>>> -		}
>>>>>>> 		return result;
>>>>>>> 	}
>>>>>>>
>>>>>>> @@ -407,9 +383,6 @@ int ustcomm_recv_fd(int sock,
>>>>>>> 			}
>>>>>>> 			cmsg = CMSG_NXTHDR(&msg, cmsg);
>>>>>>> 		}
>>>>>>> -		if (!result) {
>>>>>>> -			ERR("Failed to receive file descriptor\n");
>>>>>>> -		}
>>>>>>> 	}
>>>>>>>
>>>>>>> 	return 1;
>>>>>>> @@ -430,7 +403,6 @@ int ustcomm_send_fd(int sock,
>>>>>>> {
>>>>>>> 	struct iovec iov[2];
>>>>>>> 	struct msghdr msg;
>>>>>>> -	int result;
>>>>>>> 	struct cmsghdr *cmsg;
>>>>>>> 	char buf[CMSG_SPACE(sizeof(int))];
>>>>>>>
>>>>>>> @@ -461,11 +433,7 @@ int ustcomm_send_fd(int sock,
>>>>>>> 		msg.msg_controllen = cmsg->cmsg_len;
>>>>>>> 	}
>>>>>>>
>>>>>>> -	result = sendmsg(sock, &msg, MSG_NOSIGNAL);
>>>>>>> -	if (result < 0 && errno != EPIPE) {
>>>>>>> -		PERROR("sendmsg failed");
>>>>>>> -	}
>>>>>>> -	return result;
>>>>>>> +	return sendmsg(sock, &msg, MSG_NOSIGNAL);
>>>>>>> }
>>>>>>>
>>>>>>> int ustcomm_send(int sock,
>>>>>>> @@ -504,19 +472,16 @@ int ustcomm_connect_path(const char *name,
>>>>>>> int
>>>>>>> *connection_fd)
>>>>>>>
>>>>>>> 	fd = socket(PF_UNIX, SOCK_STREAM, 0);
>>>>>>> 	if(fd == -1) {
>>>>>>> -		PERROR("socket");
>>>>>>> 		return -1;
>>>>>>> 	}
>>>>>>>
>>>>>>> 	addr = create_sock_addr(name, &sock_addr_size);
>>>>>>> 	if (addr == NULL) {
>>>>>>> -		ERR("allocating addr failed");
>>>>>>> 		goto close_sock;
>>>>>>> 	}
>>>>>>>
>>>>>>> 	result = connect(fd, (struct sockaddr *)addr, sock_addr_size);
>>>>>>> 	if(result == -1) {
>>>>>>> -		PERROR("connect (path=%s)", name);
>>>>>>> 		goto free_sock_addr;
>>>>>>> 	}
>>>>>>>
>>>>>>> @@ -543,7 +508,6 @@ char *ustcomm_user_sock_dir(void)
>>>>>>> 	result = asprintf(&sock_dir, "%s%s", USER_SOCK_DIR,
>>>>>>> 			  cuserid(NULL));
>>>>>>> 	if (result < 0) {
>>>>>>> -		ERR("string overflow allocating directory name");
>>>>>>> 		return NULL;
>>>>>>> 	}
>>>>>>>
>>>>>>> @@ -569,14 +533,12 @@ static int connect_app_non_root(pid_t pid,
>>>>>>> int
>>>>>>> *app_fd)
>>>>>>>
>>>>>>> 	result = asprintf(&sock_name, "%s/%d", dir_name, pid);
>>>>>>> 	if (result < 0) {
>>>>>>> -		ERR("failed to allocate socket name");
>>>>>>> 		retval = -1;
>>>>>>> 		goto free_dir_name;
>>>>>>> 	}
>>>>>>>
>>>>>>> 	result = ustcomm_connect_path(sock_name, app_fd);
>>>>>>> 	if (result < 0) {
>>>>>>> -		ERR("failed to connect to app");
>>>>>>> 		retval = -1;
>>>>>>> 		goto free_sock_name;
>>>>>>> 	}
>>>>>>> @@ -657,14 +619,12 @@ int ensure_dir_exists(const char *dir,  
>>>>>>> mode_t
>>>>>>> mode)
>>>>>>>
>>>>>>> 		result = mkdir_p(dir, mode);
>>>>>>> 		if(result != 0) {
>>>>>>> -			ERR("executing in recursive creation of directory %s", dir);
>>>>>>> 			return -1;
>>>>>>> 		}
>>>>>>> 	} else {
>>>>>>> 		if (st.st_mode != mode) {
>>>>>>> 			result = chmod(dir, mode);
>>>>>>> 			if (result < 0) {
>>>>>>> -				ERR("couldn't set directory mode on %s", dir);
>>>>>>> 				return -1;
>>>>>>> 			}
>>>>>>> 		}
>>>>>>> diff --git a/libustctl/libustctl.c b/libustctl/libustctl.c
>>>>>>> index 1911434..b5d6a02 100644
>>>>>>> --- a/libustctl/libustctl.c
>>>>>>> +++ b/libustctl/libustctl.c
>>>>>>> @@ -17,6 +17,7 @@
>>>>>>> */
>>>>>>>
>>>>>>> #define _GNU_SOURCE
>>>>>>> +#include <errno.h>
>>>>>>> #include <stdio.h>
>>>>>>> #include <unistd.h>
>>>>>>> #include <getopt.h>
>>>>>>> @@ -26,8 +27,8 @@
>>>>>>> #include <dirent.h>
>>>>>>>
>>>>>>> #include "ustcomm.h"
>>>>>>> -#include "ust/ustctl.h"
>>>>>>> -#include "usterr.h"
>>>>>>> +#include <ust/ustctl.h>
>>>>>>> +#include <ust/core.h>
>>>>>>>
>>>>>>> static int do_cmd(int sock,
>>>>>>> 		  const struct ustcomm_header *req_header,
>>>>>>> @@ -57,7 +58,6 @@ static int do_cmd(int sock,
>>>>>>> 			}
>>>>>>> 		}
>>>>>>> 	} else {
>>>>>>> -		ERR("ustcomm req failed");
>>>>>>> 		if (result == 0) {
>>>>>>> 			saved_errno = ENOTCONN;
>>>>>>> 		} else {
>>>>>>> @@ -80,7 +80,6 @@ int ustctl_connect_pid(pid_t pid)
>>>>>>> 	int sock;
>>>>>>>
>>>>>>> 	if (ustcomm_connect_app(pid, &sock)) {
>>>>>>> -		ERR("could not connect to PID %u", (unsigned int) pid);
>>>>>>> 		errno = ENOTCONN;
>>>>>>> 		return -1;
>>>>>>> 	}
>>>>>>> @@ -580,20 +579,17 @@ int ustctl_get_cmsf(int sock, struct
>>>>>>> marker_status **cmsf)
>>>>>>>
>>>>>>> 	result = ustcomm_send(sock, &req_header, NULL);
>>>>>>> 	if (result <= 0) {
>>>>>>> -		PERROR("error while requesting markers list");
>>>>>>> 		return -1;
>>>>>>> 	}
>>>>>>>
>>>>>>> 	result = ustcomm_recv_alloc(sock, &res_header, &big_str);
>>>>>>> 	if (result <= 0) {
>>>>>>> -		ERR("error while receiving markers list");
>>>>>>> 		return -1;
>>>>>>> 	}
>>>>>>>
>>>>>>> 	tmp_cmsf = (struct marker_status *) zmalloc(sizeof(struct
>>>>>>> marker_status) *
>>>>>>> 						    (ustctl_count_nl(big_str) + 1));
>>>>>>> 	if (tmp_cmsf == NULL) {
>>>>>>> -		ERR("Failed to allocate CMSF array");
>>>>>>> 		return -1;
>>>>>>> 	}
>>>>>>>
>>>>>>> @@ -672,13 +668,11 @@ int ustctl_get_tes(int sock, struct
>>>>>>> trace_event_status **tes)
>>>>>>>
>>>>>>> 	result = ustcomm_send(sock, &req_header, NULL);
>>>>>>> 	if (result != 1) {
>>>>>>> -		ERR("error while requesting trace_event list");
>>>>>>> 		return -1;
>>>>>>> 	}
>>>>>>>
>>>>>>> 	result = ustcomm_recv_alloc(sock, &res_header, &big_str);
>>>>>>> 	if (result != 1) {
>>>>>>> -		ERR("error while receiving markers list");
>>>>>>> 		return -1;
>>>>>>> 	}
>>>>>>>
>>>>>>> @@ -686,7 +680,6 @@ int ustctl_get_tes(int sock, struct
>>>>>>> trace_event_status **tes)
>>>>>>> 		zmalloc(sizeof(struct trace_event_status) *
>>>>>>> 			(ustctl_count_nl(big_str) + 1));
>>>>>>> 	if (tmp_tes == NULL) {
>>>>>>> -		ERR("Failed to allocate TES array");
>>>>>>> 		return -1;
>>>>>>> 	}
>>>>>>>
>>>>>>> -- 
>>>>>>> 1.7.1
>>>>>>>
>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> ltt-dev mailing list
>>>>>>> ltt-dev at lists.casi.polymtl.ca
>>>>>>> http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev
>>>>>>>
>>>>>>
>>>>>> -- 
>>>>>> Mathieu Desnoyers
>>>>>> Operating System Efficiency R&D Consultant
>>>>>> EfficiOS Inc.
>>>>>> http://www.efficios.com
>>>>>>
>>>>>> _______________________________________________
>>>>>> ltt-dev mailing list
>>>>>> ltt-dev at lists.casi.polymtl.ca
>>>>>> http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev
>>>>>
>>>>
>>>> -- 
>>>> Mathieu Desnoyers
>>>> Operating System Efficiency R&D Consultant
>>>> EfficiOS Inc.
>>>> http://www.efficios.com
>>>
>>
>> -- 
>> Mathieu Desnoyers
>> Operating System Efficiency R&D Consultant
>> EfficiOS Inc.
>> http://www.efficios.com
>

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com