Re: [systemd-devel] [PATCH] sd-dhcp-client: log positive error number
Applied. Thanks!
On Sun, Apr 27, 2014 at 10:01 PM, Umut Tezduyar Lindskog
wrote:
> Log error no for such client_stop(client, DHCP_EVENT_STOP)
> ---
> src/libsystemd-network/sd-dhcp-client.c | 16 +++-
> 1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/src/libsystemd-network/sd-dhcp-client.c
> b/src/libsystemd-network/sd-dhcp-client.c
> index 854c671..f2266e0 100644
> --- a/src/libsystemd-network/sd-dhcp-client.c
> +++ b/src/libsystemd-network/sd-dhcp-client.c
> @@ -231,7 +231,21 @@ 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 {
> +switch(error) {
> +case DHCP_EVENT_STOP:
> +log_dhcp_client(client, "STOPPED: Requested by
> user");
> +break;
> +case DHCP_EVENT_NO_LEASE:
> +log_dhcp_client(client, "STOPPED: No lease");
> +break;
> +default:
> +log_dhcp_client(client, "STOPPED: Unknown reason");
> +break;
> +}
> +}
>
> client = client_notify(client, error);
>
> --
> 1.7.10.4
>
> ___
> systemd-devel mailing list
> systemd-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/systemd-devel
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
[systemd-devel] [PATCH] sd-dhcp-client: log positive error number
Log error no for such client_stop(client, DHCP_EVENT_STOP)
---
src/libsystemd-network/sd-dhcp-client.c | 16 +++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/src/libsystemd-network/sd-dhcp-client.c
b/src/libsystemd-network/sd-dhcp-client.c
index 854c671..f2266e0 100644
--- a/src/libsystemd-network/sd-dhcp-client.c
+++ b/src/libsystemd-network/sd-dhcp-client.c
@@ -231,7 +231,21 @@ 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 {
+switch(error) {
+case DHCP_EVENT_STOP:
+log_dhcp_client(client, "STOPPED: Requested by user");
+break;
+case DHCP_EVENT_NO_LEASE:
+log_dhcp_client(client, "STOPPED: No lease");
+break;
+default:
+log_dhcp_client(client, "STOPPED: Unknown reason");
+break;
+}
+}
client = client_notify(client, error);
--
1.7.10.4
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] sd-dhcp-client: log positive error number
On Sat, Apr 26, 2014 at 7:18 AM, Umut Tezduyar Lindskog
wrote:
> On Sat, Apr 26, 2014 at 12:36 AM, Tom Gundersen wrote:
>> On Sun, Apr 13, 2014 at 12:52 PM, Umut Tezduyar Lindskog
>> 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
Re: [systemd-devel] [PATCH] sd-dhcp-client: log positive error number
On Sat, Apr 26, 2014 at 12:36 AM, Tom Gundersen wrote:
> On Sun, Apr 13, 2014 at 12:52 PM, Umut Tezduyar Lindskog
> 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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/systemd-devel
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] sd-dhcp-client: log positive error number
On Sun, Apr 13, 2014 at 12:52 PM, Umut Tezduyar Lindskog
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
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] sd-dhcp-client: log positive error number
On Sun, Apr 13, 2014 at 6:01 PM, Tom Gundersen wrote: > On Sun, Apr 13, 2014 at 2:11 PM, Umut Tezduyar Lindskog > wrote: >> On Sun, Apr 13, 2014 at 1:16 PM, Tom Gundersen wrote: >>> Hm, why? Are not error messages more useful? >> >> What is going to be the mapping for DHCP_EVENT_STOP? >> >> log_dhcp_client(client, "STOPPED: %s", strerror(-DHCP_EVENT_STOP)); >> > > Ah, now I get it. I guess we should special case this to something like: > > if (r >= 0) > log_dhcp_client(client, "STOPPED"); > else > log_dhcp_client(client, "STOPPED: %s", strerror(-r)); > > That way we get the error messages, but avoid things like "STOPPED: > Success." as well as the problem you pointed out. > > What do you think? I can see following 2 cases of client stopping it self. client_stop(client, DHCP_EVENT_NO_LEASE); client_stop(client, DHCP_EVENT_STOP); > > -t Umut ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] sd-dhcp-client: log positive error number
On Sun, Apr 13, 2014 at 2:11 PM, Umut Tezduyar Lindskog wrote: > On Sun, Apr 13, 2014 at 1:16 PM, Tom Gundersen wrote: >> Hm, why? Are not error messages more useful? > > What is going to be the mapping for DHCP_EVENT_STOP? > > log_dhcp_client(client, "STOPPED: %s", strerror(-DHCP_EVENT_STOP)); > Ah, now I get it. I guess we should special case this to something like: if (r >= 0) log_dhcp_client(client, "STOPPED"); else log_dhcp_client(client, "STOPPED: %s", strerror(-r)); That way we get the error messages, but avoid things like "STOPPED: Success." as well as the problem you pointed out. What do you think? -t ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] sd-dhcp-client: log positive error number
On Sun, Apr 13, 2014 at 1:16 PM, Tom Gundersen wrote:
> Hm, why? Are not error messages more useful?
What is going to be the mapping for DHCP_EVENT_STOP?
log_dhcp_client(client, "STOPPED: %s", strerror(-DHCP_EVENT_STOP));
>
> On 13 Apr 2014 12:53, "Umut Tezduyar Lindskog"
> 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);
>>
>> client = client_notify(client, error);
>>
>> --
>> 1.7.10.4
>>
>> ___
>> systemd-devel mailing list
>> systemd-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/systemd-devel
>
>
> ___
> systemd-devel mailing list
> systemd-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/systemd-devel
>
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] sd-dhcp-client: log positive error number
Hm, why? Are not error messages more useful?
On 13 Apr 2014 12:53, "Umut Tezduyar Lindskog"
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);
>
> client = client_notify(client, error);
>
> --
> 1.7.10.4
>
> ___
> systemd-devel mailing list
> systemd-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/systemd-devel
>
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
[systemd-devel] [PATCH] sd-dhcp-client: log positive error number
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);
client = client_notify(client, error);
--
1.7.10.4
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
