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

[Tom Gundersen]

On Sat, Apr 26, 2014 at 7:18 AM, Umut Tezduyar Lindskog
<umut at tezduyar.com> wrote:
> 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?

Do you think this has more uses than the one place, then sure
(otherwise I don't know if it is worth it).

> dhcp_client_strerrr (int error) {
>   switch (error) {
>   case error < 0:
>       return sterror(-error)
>   case DHCP_EVENT_STOPPED:
>       return "normal" (or whatever)

Hm, what to call this... Currently it is "Success", but I think that's
weird. As is "normal". Would be best to not say anything... (but then
the colon is odd). Maybe "Requested by user" or something like that?

>   case DHCP_EVENT_NO_LEASE
>       return "no lease"
>   default
>       return "not implemented"

Better make this "unkwnown reason", or the user reading the log won't
know what was not implemented...

Better make the first letter of each string a capital, that's what
strerror() does.

Cheers,

Tom