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

[Umut Tezduyar Lindskog]

On Sat, Apr 26, 2014 at 12:36 AM, Tom Gundersen <teg at jklm.no> wrote:
> 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?

Great. Should I wrap all of this to a dhcp_client_sterror function?

dhcp_client_strerrr (int error) {
  switch (error) {
  case error < 0:
      return sterror(-error)
  case DHCP_EVENT_STOPPED:
      return "normal" (or whatever)
  case DHCP_EVENT_NO_LEASE
      return "no lease"
  default
      return "not implemented"
}

Umut

>
> Cheers,
>
> Tom
> _______________________________________________
> systemd-devel mailing list
> systemd-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/systemd-devel