From mboxrd@z Thu Jan 1 00:00:00 1970 From: compudj@krystal.dyndns.org (Mathieu Desnoyers) Date: Fri, 1 Apr 2011 17:09:25 -0400 Subject: [ltt-dev] [UST PATCH] Remove all "usterr.h" references in libusctl/ustcomm In-Reply-To: <25C28BB2-97EF-4551-8B92-587635AF2D0D@ludd.ltu.se> References: <1301668661-4064-1-git-send-email-nils.carlson@ericsson.com> <0947CF8C-9A7B-4657-84DF-728FE947CFDF@ludd.ltu.se> <25C28BB2-97EF-4551-8B92-587635AF2D0D@ludd.ltu.se> Message-ID: * 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 >>>>>>> --- >>>>>>> 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 ? >> > 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 >>>>>>> #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 > -- Mathieu Desnoyers Operating System Efficiency R&D Consultant EfficiOS Inc. http://www.efficios.com