On Sat, Apr 26, 2014 at 7:18 AM, Umut Tezduyar Lindskog
<u...@tezduyar.com> wrote:
> On Sat, Apr 26, 2014 at 12:36 AM, Tom Gundersen <t...@jklm.no> wrote:
>> On Sun, Apr 13, 2014 at 12:52 PM, Umut Tezduyar Lindskog
>> <umut.tezdu...@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
_______________________________________________
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
