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

Mathieu Desnoyers compudj at krystal.dyndns.org
Fri Apr 1 16:51:13 EDT 2011


* 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




More information about the lttng-dev mailing list