Re: [systemd-devel] [PATCH] timedatectl: work with old timedated

[Shawn Landden]

On Thu, Dec 12, 2013 at 11:02 AM, Lennart Poettering
 wrote:
> On Thu, 12.12.13 10:00, Shawn Landden (sh...@churchofgit.com) wrote:
>
>
> Applied. Dropped the fflush(stderr) bits though as we that's not
> necessary for stderr, and not even for stdout if an \n was printed
> anyway...
ok, I was worried about the lack of synchronization between stderr and
stdout causing them to interlace.
>
> Thanks!
>
>>
>> diff --git a/src/timedate/timedatectl.c b/src/timedate/timedatectl.c
>> index 9b81513..6b09559 100644
>> --- a/src/timedate/timedatectl.c
>> +++ b/src/timedate/timedatectl.c
>> @@ -98,6 +98,7 @@ static void print_status_info(const StatusInfo *i) {
>>  char s[32];
>>  struct tm tm;
>>  time_t sec;
>> +bool have_time = false;
>>  char *zc, *zn;
>>  time_t t, tc, tn;
>>  int dn;
>> @@ -109,20 +110,35 @@ static void print_status_info(const StatusInfo *i) {
>>  /* Enforce the values of /etc/localtime */
>>  if (getenv("TZ")) {
>>  fprintf(stderr, "Warning: ignoring the TZ variable, reading 
>> the system's timezone setting only.\n\n");
>> +fflush(stderr);
>>  unsetenv("TZ");
>>  }
>>
>> -sec = (time_t) (i->time / USEC_PER_SEC);
>> +if (i->time != 0) {
>> +sec = (time_t) (i->time / USEC_PER_SEC);
>> +have_time = true;
>> +} else if (arg_transport == BUS_TRANSPORT_LOCAL) {
>> +sec = time(NULL);
>> +have_time = true;
>> +} else {
>> +fprintf(stderr, "Warning: could not get time from timedated 
>> and not operating locally.\n\n");
>> +fflush(stderr);
>> +}
>>
>> -zero(tm);
>> -assert_se(strftime(a, sizeof(a), "%a %Y-%m-%d %H:%M:%S %Z", 
>> localtime_r(&sec, &tm)) > 0);
>> -char_array_0(a);
>> -printf("  Local time: %s\n", a);
>> +if (have_time) {
>> +zero(tm);
>> +assert_se(strftime(a, sizeof(a), "%a %Y-%m-%d %H:%M:%S %Z", 
>> localtime_r(&sec, &tm)) > 0);
>> +char_array_0(a);
>> +printf("  Local time: %s\n", a);
>>
>> -zero(tm);
>> -assert_se(strftime(a, sizeof(a), "%a %Y-%m-%d %H:%M:%S UTC", 
>> gmtime_r(&sec, &tm)) > 0);
>> -char_array_0(a);
>> -printf("  Universal time: %s\n", a);
>> +zero(tm);
>> +assert_se(strftime(a, sizeof(a), "%a %Y-%m-%d %H:%M:%S 
>> UTC", gmtime_r(&sec, &tm)) > 0);
>> +char_array_0(a);
>> +printf("  Universal time: %s\n", a);
>> +} else {
>> +printf("  Local time: %s\n", "n/a");
>> +printf("  Universal time: %s\n", "n/a");
>> +}
>>
>>  if (i->rtc_time > 0) {
>>  time_t rtc_sec;
>> @@ -133,7 +149,7 @@ static void print_status_info(const StatusInfo *i) {
>>  char_array_0(a);
>>  printf("RTC time: %s\n", a);
>>  } else
>> -printf("RTC time: n/a\n");
>> +printf("RTC time: %s\n", "n/a");
>>
>>  zero(tm);
>>  assert_se(strftime(a, sizeof(a), "%Z, %z", localtime_r(&sec, &tm)) 
>> > 0);
>> @@ -151,8 +167,8 @@ static void print_status_info(const StatusInfo *i) {
>>   &tc, &zc, &is_dstc,
>>   &tn, &dn, &zn, &is_dstn);
>>  if (r < 0)
>> -printf("  DST active: n/a\n");
>> -else {
>> +printf("  DST active: %s\n", "n/a");
>> +else if (have_time) {
>>  printf("  DST active: %s\n", yes_no(is_dstc));
>>
>>  t = tc - 1;
>> @@ -183,7 +199,8 @@ static void print_status_info(const StatusInfo *i) {
>>
>>  free(zc);
>>  free(zn);
>> -}
>> +} else
>> +printf("  DST active: %s\n", yes_no(is_dstc));
>>
>>  if (i->rtc_local)
>>  fputs("\n" ANSI_HIGHLIGHT_ON
>
>
> Lennart
>
> --
> Lennart Poettering, Red Hat
> ___
> 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] timedatectl: work with old timedated

[Lennart Poettering]

On Thu, 12.12.13 10:00, Shawn Landden (sh...@churchofgit.com) wrote:


Applied. Dropped the fflush(stderr) bits though as we that's not
necessary for stderr, and not even for stdout if an \n was printed
anyway...

Thanks!

> 
> diff --git a/src/timedate/timedatectl.c b/src/timedate/timedatectl.c
> index 9b81513..6b09559 100644
> --- a/src/timedate/timedatectl.c
> +++ b/src/timedate/timedatectl.c
> @@ -98,6 +98,7 @@ static void print_status_info(const StatusInfo *i) {
>  char s[32];
>  struct tm tm;
>  time_t sec;
> +bool have_time = false;
>  char *zc, *zn;
>  time_t t, tc, tn;
>  int dn;
> @@ -109,20 +110,35 @@ static void print_status_info(const StatusInfo *i) {
>  /* Enforce the values of /etc/localtime */
>  if (getenv("TZ")) {
>  fprintf(stderr, "Warning: ignoring the TZ variable, reading 
> the system's timezone setting only.\n\n");
> +fflush(stderr);
>  unsetenv("TZ");
>  }
>  
> -sec = (time_t) (i->time / USEC_PER_SEC);
> +if (i->time != 0) {
> +sec = (time_t) (i->time / USEC_PER_SEC);
> +have_time = true;
> +} else if (arg_transport == BUS_TRANSPORT_LOCAL) {
> +sec = time(NULL);
> +have_time = true;
> +} else {
> +fprintf(stderr, "Warning: could not get time from timedated 
> and not operating locally.\n\n");
> +fflush(stderr);
> +}
>  
> -zero(tm);
> -assert_se(strftime(a, sizeof(a), "%a %Y-%m-%d %H:%M:%S %Z", 
> localtime_r(&sec, &tm)) > 0);
> -char_array_0(a);
> -printf("  Local time: %s\n", a);
> +if (have_time) {
> +zero(tm);
> +assert_se(strftime(a, sizeof(a), "%a %Y-%m-%d %H:%M:%S %Z", 
> localtime_r(&sec, &tm)) > 0);
> +char_array_0(a);
> +printf("  Local time: %s\n", a);
>  
> -zero(tm);
> -assert_se(strftime(a, sizeof(a), "%a %Y-%m-%d %H:%M:%S UTC", 
> gmtime_r(&sec, &tm)) > 0);
> -char_array_0(a);
> -printf("  Universal time: %s\n", a);
> +zero(tm);
> +assert_se(strftime(a, sizeof(a), "%a %Y-%m-%d %H:%M:%S UTC", 
> gmtime_r(&sec, &tm)) > 0);
> +char_array_0(a);
> +printf("  Universal time: %s\n", a);
> +} else {
> +printf("  Local time: %s\n", "n/a");
> +printf("  Universal time: %s\n", "n/a");
> +}
>  
>  if (i->rtc_time > 0) {
>  time_t rtc_sec;
> @@ -133,7 +149,7 @@ static void print_status_info(const StatusInfo *i) {
>  char_array_0(a);
>  printf("RTC time: %s\n", a);
>  } else
> -printf("RTC time: n/a\n");
> +printf("RTC time: %s\n", "n/a");
>  
>  zero(tm);
>  assert_se(strftime(a, sizeof(a), "%Z, %z", localtime_r(&sec, &tm)) > 
> 0);
> @@ -151,8 +167,8 @@ static void print_status_info(const StatusInfo *i) {
>   &tc, &zc, &is_dstc,
>   &tn, &dn, &zn, &is_dstn);
>  if (r < 0)
> -printf("  DST active: n/a\n");
> -else {
> +printf("  DST active: %s\n", "n/a");
> +else if (have_time) {
>  printf("  DST active: %s\n", yes_no(is_dstc));
>  
>  t = tc - 1;
> @@ -183,7 +199,8 @@ static void print_status_info(const StatusInfo *i) {
>  
>  free(zc);
>  free(zn);
> -}
> +} else
> +printf("  DST active: %s\n", yes_no(is_dstc));
>  
>  if (i->rtc_local)
>  fputs("\n" ANSI_HIGHLIGHT_ON


Lennart

-- 
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH] timedatectl: work with old timedated

[Lennart Poettering]

On Wed, 11.12.13 14:04, Shawn Landden (sh...@churchofgit.com) wrote:

> Which does have TimeUSec. Should we specifically check for this method
> instead of assuming time=0 means it doesn't exist?

Sounds OK to just check against time == 0.

>  
> +if (i->time)

For numeric values please always check with explicit numerical
comparison. Something like this:

if (i->time > 0) 

or so..

> +sec = (time_t) (i->time / USEC_PER_SEC);
> +else if (arg_transport == BUS_TRANSPORT_LOCAL)
> +sec = time(NULL);
> +else
> +return (void)fprintf(stderr, "Could not get time from
> timedated and not operating locally.\n\n");

I am pretty sure we shouldn't exit here. We should instead show a line
of "n/a" or so for this value. 

Also please do not do stuff like "return (void) printf()"... We are not
in a contest to write non-obvious C code

Lennart

-- 
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel