<p dir="ltr">I think this is probably pretty good. most of those just look like the programmer being lazy and not remembering wl_log exists. We may want to keep it as printf in the cases where it immediately aborts or calls assert(0). That way it guarantees a message gets printed in the case where it causes the program to die.</p>

<p dir="ltr">That said, we may want to make another log function called wl_error or wl_fatal for actual fatal errors that internally does an assert(0).  If we did add such a function it might want to have it's own log handler.</p>

<p dir="ltr">Kristian,<br>
Do you have any thoughts?</p>
<p dir="ltr">Thanks,<br>
--Jason Ekstrand</p>
<div class="gmail_quote">On Sep 12, 2013 9:36 PM, "Chang Liu" <<a href="mailto:cl91tp@gmail.com">cl91tp@gmail.com</a>> wrote:<br type="attribution"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
use wl_log instead of printf and fprintf in core library<br>
---<br>
I'm pretty sure these printf usages should be avoided since libraries should<br>
not print to stdout. But I'm not sure if there is any reason for favoring a<br>
fprintf to stderr over wl_log. Please enlighten me if there is any.<br>
<br>
 src/connection.c     | 36 ++++++++++++++++++------------------<br>
 src/event-loop.c     |  8 ++++----<br>
 src/wayland-client.c | 14 ++++++--------<br>
 3 files changed, 28 insertions(+), 30 deletions(-)<br>
<br>
diff --git a/src/connection.c b/src/connection.c<br>
index 451b93e..40b7fbd 100644<br>
--- a/src/connection.c<br>
+++ b/src/connection.c<br>
@@ -512,7 +512,7 @@ wl_closure_marshal(struct wl_object *sender, uint32_t opcode,<br>
<br>
        count = arg_count_for_signature(message->signature);<br>
        if (count > WL_CLOSURE_MAX_ARGS) {<br>
-               printf("too many args (%d)\n", count);<br>
+               wl_log("too many args (%d)\n", count);<br>
                errno = EINVAL;<br>
                return NULL;<br>
        }<br>
@@ -557,14 +557,14 @@ wl_closure_marshal(struct wl_object *sender, uint32_t opcode,<br>
                        fd = args[i].h;<br>
                        dup_fd = wl_os_dupfd_cloexec(fd, 0);<br>
                        if (dup_fd < 0) {<br>
-                               fprintf(stderr, "dup failed: %m");<br>
+                               wl_log("dup failed: %m");<br>
                                abort();<br>
                        }<br>
                        closure->args[i].h = dup_fd;<br>
                        break;<br>
                default:<br>
-                       fprintf(stderr, "unhandled format code: '%c'\n",<br>
-                               arg.type);<br>
+                       wl_log("unhandled format code: '%c'\n",<br>
+                              arg.type);<br>
                        assert(0);<br>
                        break;<br>
                }<br>
@@ -615,7 +615,7 @@ wl_connection_demarshal(struct wl_connection *connection,<br>
<br>
        count = arg_count_for_signature(message->signature);<br>
        if (count > WL_CLOSURE_MAX_ARGS) {<br>
-               printf("too many args (%d)\n", count);<br>
+               wl_log("too many args (%d)\n", count);<br>
                errno = EINVAL;<br>
                wl_connection_consume(connection, size);<br>
                return NULL;<br>
@@ -642,7 +642,7 @@ wl_connection_demarshal(struct wl_connection *connection,<br>
                signature = get_next_argument(signature, &arg);<br>
<br>
                if (arg.type != 'h' && p + 1 > end) {<br>
-                       printf("message too short, "<br>
+                       wl_log("message too short, "<br>
                               "object (%d), message %s(%s)\n",<br>
                               *p, message->name, message->signature);<br>
                        errno = EINVAL;<br>
@@ -669,7 +669,7 @@ wl_connection_demarshal(struct wl_connection *connection,<br>
<br>
                        next = p + DIV_ROUNDUP(length, sizeof *p);<br>
                        if (next > end) {<br>
-                               printf("message too short, "<br>
+                               wl_log("message too short, "<br>
                                       "object (%d), message %s(%s)\n",<br>
                                       closure->sender_id, message->name,<br>
                                       message->signature);<br>
@@ -680,7 +680,7 @@ wl_connection_demarshal(struct wl_connection *connection,<br>
                        s = (char *) p;<br>
<br>
                        if (length > 0 && s[length - 1] != '\0') {<br>
-                               printf("string not nul-terminated, "<br>
+                               wl_log("string not nul-terminated, "<br>
                                       "message %s(%s)\n",<br>
                                       message->name, message->signature);<br>
                                errno = EINVAL;<br>
@@ -695,7 +695,7 @@ wl_connection_demarshal(struct wl_connection *connection,<br>
                        closure->args[i].n = id;<br>
<br>
                        if (id == 0 && !arg.nullable) {<br>
-                               printf("NULL object received on non-nullable "<br>
+                               wl_log("NULL object received on non-nullable "<br>
                                       "type, message %s(%s)\n", message->name,<br>
                                       message->signature);<br>
                                errno = EINVAL;<br>
@@ -707,7 +707,7 @@ wl_connection_demarshal(struct wl_connection *connection,<br>
                        closure->args[i].n = id;<br>
<br>
                        if (id == 0 && !arg.nullable) {<br>
-                               printf("NULL new ID received on non-nullable "<br>
+                               wl_log("NULL new ID received on non-nullable "<br>
                                       "type, message %s(%s)\n", message->name,<br>
                                       message->signature);<br>
                                errno = EINVAL;<br>
@@ -715,7 +715,7 @@ wl_connection_demarshal(struct wl_connection *connection,<br>
                        }<br>
<br>
                        if (wl_map_reserve_new(objects, id) < 0) {<br>
-                               printf("not a valid new object id (%d), "<br>
+                               wl_log("not a valid new object id (%d), "<br>
                                       "message %s(%s)\n",<br>
                                       id, message->name, message->signature);<br>
                                errno = EINVAL;<br>
@@ -728,7 +728,7 @@ wl_connection_demarshal(struct wl_connection *connection,<br>
<br>
                        next = p + DIV_ROUNDUP(length, sizeof *p);<br>
                        if (next > end) {<br>
-                               printf("message too short, "<br>
+                               wl_log("message too short, "<br>
                                       "object (%d), message %s(%s)\n",<br>
                                       closure->sender_id, message->name,<br>
                                       message->signature);<br>
@@ -749,7 +749,7 @@ wl_connection_demarshal(struct wl_connection *connection,<br>
                        closure->args[i].h = fd;<br>
                        break;<br>
                default:<br>
-                       printf("unknown type\n");<br>
+                       wl_log("unknown type\n");<br>
                        assert(0);<br>
                        break;<br>
                }<br>
@@ -808,7 +808,7 @@ wl_closure_lookup_objects(struct wl_closure *closure, struct wl_map *objects)<br>
                                 * destroyed client side */<br>
                                object = NULL;<br>
                        } else if (object == NULL && id != 0) {<br>
-                               printf("unknown object (%u), message %s(%s)\n",<br>
+                               wl_log("unknown object (%u), message %s(%s)\n",<br>
                                       id, message->name, message->signature);<br>
                                object = NULL;<br>
                                errno = EINVAL;<br>
@@ -818,7 +818,7 @@ wl_closure_lookup_objects(struct wl_closure *closure, struct wl_map *objects)<br>
                        if (object != NULL && message->types[i] != NULL &&<br>
                            !wl_interface_equal((object)->interface,<br>
                                                message->types[i])) {<br>
-                               printf("invalid object (%u), type (%s), "<br>
+                               wl_log("invalid object (%u), type (%s), "<br>
                                       "message %s(%s)\n",<br>
                                       id, (object)->interface->name,<br>
                                       message->name, message->signature);<br>
@@ -884,7 +884,7 @@ convert_arguments_to_ffi(const char *signature, uint32_t flags,<br>
                        ffi_args[i] = &args[i].h;<br>
                        break;<br>
                default:<br>
-                       printf("unknown type\n");<br>
+                       wl_log("unknown type\n");<br>
                        assert(0);<br>
                        break;<br>
                }<br>
@@ -944,8 +944,8 @@ copy_fds_to_connection(struct wl_closure *closure,<br>
<br>
                fd = closure->args[i].h;<br>
                if (wl_connection_put_fd(connection, fd)) {<br>
-                       fprintf(stderr, "request could not be marshaled: "<br>
-                               "can't send file descriptor");<br>
+                       wl_log("request could not be marshaled: "<br>
+                              "can't send file descriptor");<br>
                        return -1;<br>
                }<br>
        }<br>
diff --git a/src/event-loop.c b/src/event-loop.c<br>
index d323601..43d4f50 100644<br>
--- a/src/event-loop.c<br>
+++ b/src/event-loop.c<br>
@@ -97,7 +97,7 @@ add_source(struct wl_event_loop *loop,<br>
        struct epoll_event ep;<br>
<br>
        if (source->fd < 0) {<br>
-               fprintf(stderr, "could not add source\n: %m");<br>
+               wl_log("could not add source\n: %m");<br>
                free(source);<br>
                return NULL;<br>
        }<br>
@@ -175,7 +175,7 @@ wl_event_source_timer_dispatch(struct wl_event_source *source,<br>
        len = read(source->fd, &expires, sizeof expires);<br>
        if (len != sizeof expires)<br>
                /* Is there anything we can do here?  Will this ever happen? */<br>
-               fprintf(stderr, "timerfd read error: %m\n");<br>
+               wl_log("timerfd read error: %m\n");<br>
<br>
        return timer_source->func(timer_source->base.data);<br>
 }<br>
@@ -212,7 +212,7 @@ wl_event_source_timer_update(struct wl_event_source *source, int ms_delay)<br>
        its.it_value.tv_sec = ms_delay / 1000;<br>
        its.it_value.tv_nsec = (ms_delay % 1000) * 1000 * 1000;<br>
        if (timerfd_settime(source->fd, 0, &its, NULL) < 0) {<br>
-               fprintf(stderr, "could not set timerfd\n: %m");<br>
+               wl_log("could not set timerfd\n: %m");<br>
                return -1;<br>
        }<br>
<br>
@@ -237,7 +237,7 @@ wl_event_source_signal_dispatch(struct wl_event_source *source,<br>
        len = read(source->fd, &signal_info, sizeof signal_info);<br>
        if (len != sizeof signal_info)<br>
                /* Is there anything we can do here?  Will this ever happen? */<br>
-               fprintf(stderr, "signalfd read error: %m\n");<br>
+               wl_log("signalfd read error: %m\n");<br>
<br>
        return signal_source->func(signal_source->signal_number,<br>
                                   signal_source->base.data);<br>
diff --git a/src/wayland-client.c b/src/wayland-client.c<br>
index 04d988b..9633862 100644<br>
--- a/src/wayland-client.c<br>
+++ b/src/wayland-client.c<br>
@@ -318,7 +318,7 @@ wl_proxy_add_listener(struct wl_proxy *proxy,<br>
                      void (**implementation)(void), void *data)<br>
 {<br>
        if (proxy->object.implementation || proxy->dispatcher) {<br>
-               fprintf(stderr, "proxy already has listener\n");<br>
+               wl_log("proxy already has listener\n");<br>
                return -1;<br>
        }<br>
<br>
@@ -371,7 +371,7 @@ wl_proxy_add_dispatcher(struct wl_proxy *proxy,<br>
                        const void *implementation, void *data)<br>
 {<br>
        if (proxy->object.implementation || proxy->dispatcher) {<br>
-               fprintf(stderr, "proxy already has listener\n");<br>
+               wl_log("proxy already has listener\n");<br>
                return -1;<br>
        }<br>
<br>
@@ -453,7 +453,7 @@ wl_proxy_marshal_array(struct wl_proxy *proxy, uint32_t opcode,<br>
                                     &proxy->object.interface->methods[opcode]);<br>
<br>
        if (closure == NULL) {<br>
-               fprintf(stderr, "Error marshalling request\n");<br>
+               wl_log("error marshalling request\n");<br>
                abort();<br>
        }<br>
<br>
@@ -461,7 +461,7 @@ wl_proxy_marshal_array(struct wl_proxy *proxy, uint32_t opcode,<br>
                wl_closure_print(closure, &proxy->object, true);<br>
<br>
        if (wl_closure_send(closure, proxy->display->connection)) {<br>
-               fprintf(stderr, "Error sending request: %m\n");<br>
+               wl_log("error sending request: %m\n");<br>
                abort();<br>
        }<br>
<br>
@@ -532,8 +532,7 @@ connect_to_socket(const char *name)<br>
<br>
        runtime_dir = getenv("XDG_RUNTIME_DIR");<br>
        if (!runtime_dir) {<br>
-               fprintf(stderr,<br>
-                       "error: XDG_RUNTIME_DIR not set in the environment.\n");<br>
+               wl_log("error: XDG_RUNTIME_DIR not set in the environment.\n");<br>
<br>
                /* to prevent programs reporting<br>
                 * "failed to create display: Success" */<br>
@@ -558,8 +557,7 @@ connect_to_socket(const char *name)<br>
<br>
        assert(name_size > 0);<br>
        if (name_size > (int)sizeof addr.sun_path) {<br>
-               fprintf(stderr,<br>
-                      "error: socket path \"%s/%s\" plus null terminator"<br>
+               wl_log("error: socket path \"%s/%s\" plus null terminator"<br>
                       " exceeds 108 bytes\n", runtime_dir, name);<br>
                close(fd);<br>
                /* to prevent programs reporting<br>
--<br>
1.8.3.4<br>
<br>
_______________________________________________<br>
wayland-devel mailing list<br>
<a href="mailto:wayland-devel@lists.freedesktop.org">wayland-devel@lists.freedesktop.org</a><br>
<a href="http://lists.freedesktop.org/mailman/listinfo/wayland-devel" target="_blank">http://lists.freedesktop.org/mailman/listinfo/wayland-devel</a><br>
</blockquote></div>