[ltt-dev] [UST PATCH] Remove all "usterr.h" references in libusctl/ustcomm
Nils Carlson
nils.carlson at ludd.ltu.se
Fri Apr 1 16:45:58 EDT 2011
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
More information about the lttng-dev
mailing list