[systemd-devel] [PATCH 01/12] fsckd daemon for inter-fsckd communication

[Lennart Poettering]

On Thu, 29.01.15 18:41, Didier Roche (didrocks at ubuntu.com) wrote:

> Le 28/01/2015 21:16, Lennart Poettering a écrit :
> 
> Thanks for your feedbacks! Changes integrated, some remaining questions
> though:
> 
> >On Wed, 28.01.15 14:20, Didier Roche (didier.roche at canonical.com) wrote:
> >
> >>+        last_activity = now(CLOCK_MONOTONIC);
> >>+
> >>+        for (;;) {
> >>+                int new_client_fd = 0;
> >>+                Clients *current;
> >>+                _cleanup_free_ char *console_message = NULL;
> >>+                double current_percent = 100;
> >>+                int current_numdevices = 0, m = 0;
> >>+                usec_t t;
> >>+
> >>+                /* Initialize and list new clients */
> >>+                new_client_fd = accept4(socket_fd, NULL, NULL, SOCK_NONBLOCK);
> >>+                if (new_client_fd > 0) {
> >>+                        log_debug("new fsck client connected to fd: %d", new_client_fd);
> >>+                        current = alloca0(sizeof(Clients));
> >It's not OK to invoke alloca() in loops. This cannot work. Please use
> >normal heap memoy for this.
> 
> Oh, good to know, replaced with a regular malloc. I didn't handle the
> freeing of Clients as they are destructed on program close, should I handle
> this (and thus looping over and freeing the new char* - which part of the
> struct)?

Yes, please, always release all memory you allocate before
exiting. This makes life much much easier when valgrinding
things. There are only very few cases where it is OK to leave memory
unfreed on shutdown.

> >+                        current = NULL;
> >+                }
> >+
> >+                LIST_FOREACH(clients, current, first) {
> >+                       FsckProgress fsck_data;
> >+                       int rc = 0;
> >+
> >+                        if (current->fd > 0)
> >+                                rc = recv(current->fd, &fsck_data, sizeof(FsckProgress), 0);
> >+
> >+                        if ((current->fd != 0) && (rc == 0)) {
> >+                                log_debug("fsck client connected to
> >fd %d disconnected", current->fd);
> >Please print propery exit codes.
> 
> That was my bad, it's r rather then rc, so not a return code but number of
> bytes read, or I missing something?

Sorry, I meant "proper error codes". I.e. print "errno". In systemd we
have this idiom of:

        log_debug_errno(errno, "foo bar waldo: %m");

i.e. the log_debug_errno() macro takes the error number as first arg,
and then resolves it to a human readable string with "%m".

Lennart

-- 
Lennart Poettering, Red Hat