[systemd-devel] [PATCH] systemctl: fix and refactor wait_for_jobs

[Thomas H.P. Andersen]

I was a bit unsure what to do here. We can be waiting for multiple
jobs that each can fail. We could return an error on the first failure
but then we would not call log_error for the remaining issues. The
question is then what to return in the case of multiple errors? With
this patch (and the behavior before the sd-bus port) the last error
encountered is returned as we just overwrite the value in r.


On Sat, Dec 7, 2013 at 1:15 PM, Thomas H.P. Andersen <phomes at gmail.com> wrote:
> From: Thomas Hindoe Paaboel Andersen <phomes at gmail.com>
>
> wait_for_jobs was ignoring the errors from the jobs stored in r.
> It would only ever return whether the call to sd_bus_remove_filter
> went ok. This patch changes it to return the last job related error
> encountered. The result of the cleanup call to sd_bus_remove_filter
> is ignored.
>
> wait_for_jobs was a bit hard to read so I split it up to avoid
> the goto and deep nesting.
> ---
>  src/systemctl/systemctl.c | 89 +++++++++++++++++++++++++++++------------------
>  1 file changed, 55 insertions(+), 34 deletions(-)
>
> diff --git a/src/systemctl/systemctl.c b/src/systemctl/systemctl.c
> index 6b6cb3d..e7e3fc3 100644
> --- a/src/systemctl/systemctl.c
> +++ b/src/systemctl/systemctl.c
> @@ -1803,9 +1803,52 @@ static int enable_wait_for_jobs(sd_bus *bus) {
>          return 0;
>  }
>
> +static int bus_process_wait(sd_bus *bus) {
> +        int r;
> +
> +        for (;;) {
> +                r = sd_bus_process(bus, NULL);
> +                if (r < 0)
> +                        return r;
> +                if (r > 0)
> +                        return 0;
> +                r = sd_bus_wait(bus, (uint64_t) -1);
> +                if (r < 0)
> +                        return r;
> +        }
> +}
> +
> +static int check_wait_response(WaitData *d) {
> +        int r = 0;
> +
> +        assert(d->result);
> +
> +        if (!arg_quiet) {
> +                if (streq(d->result, "timeout"))
> +                        log_error("Job for %s timed out.", strna(d->name));
> +                else if (streq(d->result, "canceled"))
> +                        log_error("Job for %s canceled.", strna(d->name));
> +                else if (streq(d->result, "dependency"))
> +                        log_error("A dependency job for %s failed. See 'journalctl -xn' for details.", strna(d->name));
> +                else if (!streq(d->result, "done") && !streq(d->result, "skipped"))
> +                        log_error("Job for %s failed. See 'systemctl status %s' and 'journalctl -xn' for details.", strna(d->name), strna(d->name));
> +        }
> +
> +        if (streq(d->result, "timeout"))
> +                r = -ETIME;
> +        else if (streq(d->result, "canceled"))
> +                r = -ECANCELED;
> +        else if (streq(d->result, "dependency"))
> +                r = -EIO;
> +        else if (!streq(d->result, "done") && !streq(d->result, "skipped"))
> +                r = -EIO;
> +
> +        return r;
> +}
> +
>  static int wait_for_jobs(sd_bus *bus, Set *s) {
>          WaitData d = { .set = s };
> -        int r;
> +        int r = 0, q;
>
>          assert(bus);
>          assert(s);
> @@ -1815,47 +1858,25 @@ static int wait_for_jobs(sd_bus *bus, Set *s) {
>                  return log_oom();
>
>          while (!set_isempty(s)) {
> -                for (;;) {
> -                        r = sd_bus_process(bus, NULL);
> -                        if (r < 0)
> -                                return r;
> -                        if (r > 0)
> -                                break;
> -                        r = sd_bus_wait(bus, (uint64_t) -1);
> -                        if (r < 0)
> -                                return r;
> -                }
> -
> -                if (!d.result)
> -                        goto free_name;
> +                q = bus_process_wait(bus);
> +                if (q < 0)
> +                        return q;
>
> -                if (!arg_quiet) {
> -                        if (streq(d.result, "timeout"))
> -                                log_error("Job for %s timed out.", strna(d.name));
> -                        else if (streq(d.result, "canceled"))
> -                                log_error("Job for %s canceled.", strna(d.name));
> -                        else if (streq(d.result, "dependency"))
> -                                log_error("A dependency job for %s failed. See 'journalctl -xn' for details.", strna(d.name));
> -                        else if (!streq(d.result, "done") && !streq(d.result, "skipped"))
> -                                log_error("Job for %s failed. See 'systemctl status %s' and 'journalctl -xn' for details.", strna(d.name), strna(d.name));
> +                if (d.result) {
> +                        q = check_wait_response(&d);
> +                        if (q < 0)
> +                                r = q;
>                  }
>
> -                if (streq_ptr(d.result, "timeout"))
> -                        r = -ETIME;
> -                else if (streq_ptr(d.result, "canceled"))
> -                        r = -ECANCELED;
> -                else if (!streq_ptr(d.result, "done") && !streq_ptr(d.result, "skipped"))
> -                        r = -EIO;
> +                free(d.name);
> +                d.name = NULL;
>
>                  free(d.result);
>                  d.result = NULL;
> -
> -        free_name:
> -                free(d.name);
> -                d.name = NULL;
>          }
>
> -        return sd_bus_remove_filter(bus, wait_filter, &d);
> +        sd_bus_remove_filter(bus, wait_filter, &d);
> +        return r;
>  }
>
>  static int check_one_unit(sd_bus *bus, const char *name, const char *good_states, bool quiet) {
> --
> 1.8.4.2
>