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