From mboxrd@z Thu Jan 1 00:00:00 1970 From: nils.carlson@ludd.ltu.se (Nils Carlson) Date: Fri, 1 Apr 2011 22:45:58 +0200 Subject: [ltt-dev] [UST PATCH] Remove all "usterr.h" references in libusctl/ustcomm In-Reply-To: References: <1301668661-4064-1-git-send-email-nils.carlson@ericsson.com> <0947CF8C-9A7B-4657-84DF-728FE947CFDF@ludd.ltu.se> Message-ID: 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 >>>> --- >>>> 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 >>>> #include >>>> >>>> +#include >>>> #include >>>> #include >>>> #include >>>> #include >>>> >>>> #include "ustcomm.h" >>>> -#include "usterr.h" >>>> +#include >>>> #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 >>>> #include >>>> #include >>>> #include >>>> @@ -26,8 +27,8 @@ >>>> #include >>>> >>>> #include "ustcomm.h" >>>> -#include "ust/ustctl.h" >>>> -#include "usterr.h" >>>> +#include >>>> +#include >>>> >>>> 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