[systemd-devel] [PATCH] sd-dhcp-client: log positive error number

[Tom Gundersen]

On Sun, Apr 13, 2014 at 12:52 PM, Umut Tezduyar Lindskog
<umut.tezduyar at axis.com> wrote:
> Log error no for such client_stop(client, DHCP_EVENT_STOP)
> ---
>  src/libsystemd-network/sd-dhcp-client.c |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/src/libsystemd-network/sd-dhcp-client.c b/src/libsystemd-network/sd-dhcp-client.c
> index 4892203..9715953 100644
> --- a/src/libsystemd-network/sd-dhcp-client.c
> +++ b/src/libsystemd-network/sd-dhcp-client.c
> @@ -230,7 +230,10 @@ static int client_initialize(sd_dhcp_client *client) {
>  static sd_dhcp_client *client_stop(sd_dhcp_client *client, int error) {
>          assert_return(client, NULL);
>
> -        log_dhcp_client(client, "STOPPED: %s", strerror(-error));
> +        if (error < 0)
> +                log_dhcp_client(client, "STOPPED: %s", strerror(-error));
> +        else
> +                log_dhcp_client(client, "STOPPED: %d", error);

The idea is good, but I don't think the error messages when error >= 0
are going to be very helpful (just a number does not tell much). I
suggest doing a switch statement with proper logging depending on the
case. Something like (not even compile-tested):

else {
        switch(error) {
        case DHCP_EVENT_STOPPED:
                     log_dhcp_client(client, "STOPPED");
        case DHCP_EVENT_NO_LEASE:
                     log_dhcp_client(client, "STOPPED: no lease");
        default:
                     log_dhcp_client(client, "STOPPED: unknown reason");
        }
}

What do you think?

Cheers,

Tom