From mboxrd@z Thu Jan 1 00:00:00 1970 From: compudj@krystal.dyndns.org (Mathieu Desnoyers) Date: Fri, 1 Apr 2011 16:51:13 -0400 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: * 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 >>>>> --- >>>>> 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? 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 >>>>> #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 > -- Mathieu Desnoyers Operating System Efficiency R&D Consultant EfficiOS Inc. http://www.efficios.com