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

[Zbigniew Jędrzejewski-Szmek]

On Wed, Feb 04, 2015 at 05:06:45PM +0100, Didier Roche wrote:
> +typedef struct Clients {
> +        struct Manager *manager;
> +        int fd;
> +        dev_t devnum;
> +        size_t cur;
> +        size_t max;
> +        int pass;
> +        double percent;
> +        size_t buflen;
> +
> +        LIST_FIELDS(struct Clients, clients);
> +} Clients;
Would be better to call this Client.

> +typedef struct Manager {
> +        sd_event *event;
> +        Clients *clients;
But still 'Client *clients' here.

> +        if ((fabs(current_percent - m->percent) > 0.01) || (current_numdevices != m->numdevices)) {
Again, too much parens.

> +                client = malloc(sizeof(Clients));
new0(Client, 1)

(Unless you initialize all fields, it's better to zero out the structure.)

> +                if (!client)
> +                        return log_oom();
> +                client->fd = new_client_fd;
> +                client->manager = m;
> +                LIST_PREPEND(clients, m->clients, client);
> +                r = sd_event_add_io(m->event, NULL, client->fd, EPOLLIN, progress_handler, client);
> +                if (r < 0) {
> +                        remove_client(&(m->clients), client);
> +                        return r;
> +                }
I think you can do this in opposite order, and then the clean-up will
not be necessary.

> +        m = new0(Manager, 1);
> +        if (!m)
> +                return -ENOMEM;
> +
> +        r = sd_event_default(&m->event);
> +        if (r < 0)
> +                return r;
> +
> +        m->clients = NULL;
> +        m->connection_fd = fd;
> +
> +        m->console = fopen("/dev/console", "we");
> +        if (!m->console)
> +                return log_error_errno(errno, "Can't connect to /dev/console: %m");
> +        m->percent = 100;
> +        m->numdevices = 0;
> +        m->clear = 0;
Since structure is initialized to zero, there's no need to set stuff to 0 again.

> +typedef struct FsckProgress {
> +        dev_t devnum;
> +        unsigned long cur;
> +        unsigned long max;
> +        int pass;
> +} FsckProgress;
I think I asked about that before, but not 100% sure: shouldn't those be size_t?
If they are disk offsets, than long is not enough.

Zbyszek