Re: hwclock's synchronize_to_clock_tick_rtc returns inconsistent values

[J William Piggott <elseifthen@gmx.com>]

On 04/20/2015 08:21 AM, Karel Zak wrote:
> On Sun, Apr 19, 2015 at 08:16:08PM -0400, J William Piggott wrote:
>>>                         rc = select(rtc_fd + 1, &rfds, NULL, NULL, &tv);
>>>                         ret = 1;
>>>                         if (rc == -1)
>>>                                 warn(_("select() to %s to wait for clock tick
>>> failed"),
>>>                                      rtc_dev_name);
>>>                         else if (rc == 0 && debug)
>>>                                 printf(_("select() to %s to wait for clock
>>> tick timed out"),
>>>                                      rtc_dev_name);
>>>                         else
>>>                                 ret = 0;
>>>
>>
>> Karel,
>>
>> The select time out still needs to return 1, so the debug test should
>> have been a separate statement.
> 
> Yes, the debug (-D) should bot affect how this code works, but it
> seems that hwclock.c:manipulate_clock() assumes return 2 after 
> time out and we already use "2" in busy wait version of the
> synchronization. 
> 

Returning 2 will not fix this bug. When select times out, we need to
error out on everything except set functions (even that exception has
issues, because we need to also inhibit updating the drift factor then.
Nobody has complained about that so it is not a priority to fix. I will
look at it when refactoring).

hwclock.c:1326 should not be testing for "rc != 2", but that is for
Alpha so I am concerned about changing it now either.

Previous to the commit that caused this regression, the behavior was to
return 1, that was correct.

Removing the 'dead' code should be a separate commit from this
regression fix, IMHO.

I do not think two message strings justify using a switch, it is
inconsistent with the style of the rest of the code. I would just
separate the debug test for now:

diff --git a/sys-utils/hwclock-rtc.c b/sys-utils/hwclock-rtc.c
index 78f42aa..b5ecc5a 100644
--- a/sys-utils/hwclock-rtc.c
+++ b/sys-utils/hwclock-rtc.c
@@ -313,10 +313,11 @@ static int synchronize_to_clock_tick_rtc(void)
                        if (rc == -1)
                                warn(_("select() to %s to wait for clock tick failed"),
                                     rtc_dev_name);
-                       else if (rc == 0 && debug)
-                               printf(_("select() to %s to wait for clock tick timed out"),
-                                    rtc_dev_name);
-                       else
+                       else if (rc == 0) {
+                               if (debug)
+                                       printf(_("select() to %s to wait for clock tick timed out"),
+                                              rtc_dev_name);
+                       } else
                                ret = 0;
 #endif
 


I did some basic hwclock testing with the above patch, but I didn't come
up with a quick way to force the select time out to test this specific
change.


>> I don't have time to patch and test this right now, but I can do it
>> later if you want?
> 
> See the patch below (note that patch also remove never used #ifdef
> dead code).
> 
> 
> diff --git a/sys-utils/hwclock-rtc.c b/sys-utils/hwclock-rtc.c
> index 78f42aa..3173591 100644
> --- a/sys-utils/hwclock-rtc.c
> +++ b/sys-utils/hwclock-rtc.c
> @@ -280,18 +280,6 @@ static int synchronize_to_clock_tick_rtc(void)
>  				       rtc_dev_name);
>  			ret = busywait_for_rtc_clock_tick(rtc_fd);
>  		} else if (rc == 0) {
> -#ifdef Wait_until_update_interrupt
> -			unsigned long dummy;
> -
> -			/* this blocks until the next update interrupt */
> -			rc = read(rtc_fd, &dummy, sizeof(dummy));
> -			ret = 1;
> -			if (rc == -1)
> -				warn(_("read() to %s to wait for clock tick failed"),
> -				     rtc_dev_name);
> -			else
> -				ret = 0;
> -#else
>  			/*
>  			 * Just reading rtc_fd fails on broken hardware: no
>  			 * update interrupt comes and a bootscript with a
> @@ -310,15 +298,22 @@ static int synchronize_to_clock_tick_rtc(void)
>  			tv.tv_usec = 0;
>  			rc = select(rtc_fd + 1, &rfds, NULL, NULL, &tv);
>  			ret = 1;
> -			if (rc == -1)
> +
> +			switch (rc) {
> +			case -1: /* error */
>  				warn(_("select() to %s to wait for clock tick failed"),
>  				     rtc_dev_name);
> -			else if (rc == 0 && debug)
> -				printf(_("select() to %s to wait for clock tick timed out"),
> -				     rtc_dev_name);
> -			else
> +				break;
> +			case 0: /* timeout */
> +				if (debug)
> +					printf(_("select() to %s to wait for clock tick timed out"),
> +					     rtc_dev_name);
> +				ret = 2;
> +				break;
> +			default: /* success */
>  				ret = 0;
> -#endif
> +				break;
> +			}
>  
>  			/* Turn off update interrupts */
>  			rc = ioctl(rtc_fd, RTC_UIE_OFF, 0);
>