gustavonihei commented on a change in pull request #4270:
URL: https://github.com/apache/incubator-nuttx/pull/4270#discussion_r682594880
##########
File path: libs/libc/time/lib_localtime.c
##########
@@ -2528,9 +2537,20 @@ void tzset(void)
{
FAR const char *name;
- tz_semtake(&g_lcl_sem);
-
name = getenv("TZ");
+ if (g_lcl_isset > 0 && name && strcmp(g_lcl_tzname, name) == 0)
+ {
+ return;
+ }
Review comment:
Thanks for providing this example scenario. I am still having a hard
time to understand how the first check copes with this concurrency between
threads.
Let's assume threads A and B.
Thread A fails the check (which means timezone info was still not
initialized). But, the OS interrupts thread A before it locks the semaphore.
Thread B then calls `tzset` and locks the semaphore, completes the
initialization of timezone info and unlock the semaphore.
Thread A later resumes execution, finally locks the semaphore and, as
expected, finds that timezone is already initialized.
The same outcome would happen if the first check did not exist, wouldn't it?
Sorry if there is something too obvious I am not realizing, but I'd like to
be convinced that the first check is not useless.
##########
File path: libs/libc/time/lib_localtime.c
##########
@@ -2528,9 +2537,20 @@ void tzset(void)
{
FAR const char *name;
- tz_semtake(&g_lcl_sem);
-
name = getenv("TZ");
+ if (g_lcl_isset > 0 && name && strcmp(g_lcl_tzname, name) == 0)
+ {
+ return;
+ }
Review comment:
Thanks for providing this example scenario. I am still having a hard
time to understand how the first check copes with this concurrency between
threads.
Let's assume threads **A** and **B**.
Thread **A** fails the check (which means timezone info was still not
initialized). But, the OS interrupts Thread **A** before it locks the semaphore.
Thread **B** then calls `tzset` and locks the semaphore, completes the
initialization of timezone info and unlock the semaphore.
Thread **A** later resumes execution, finally locks the semaphore and, as
expected, finds that timezone is already initialized.
The same outcome would happen if the first check did not exist, wouldn't it?
Sorry if there is something too obvious I am not realizing, but I'd like to
be convinced that the first check is not useless.
##########
File path: libs/libc/time/lib_localtime.c
##########
@@ -2528,9 +2537,20 @@ void tzset(void)
{
FAR const char *name;
- tz_semtake(&g_lcl_sem);
-
name = getenv("TZ");
+ if (g_lcl_isset > 0 && name && strcmp(g_lcl_tzname, name) == 0)
+ {
+ return;
+ }
Review comment:
Thanks for providing this example scenario. I am still having a hard
time to understand how the first check copes with this concurrency between
threads.
Let's assume threads **A** and **B**.
Thread **A** fails the check (which means timezone info was still not
initialized). But, the OS interrupts Thread **A** before it locks the semaphore.
Thread **B** then calls `tzset` and locks the semaphore, completes the
initialization of timezone info and unlock the semaphore.
Thread **A** later resumes execution, finally locks the semaphore and, as
expected, finds that timezone is already initialized.
The same outcome would happen if the first check did not exist, wouldn't it?
Sorry if there is something too obvious I am not realizing, but I'd like to
be convinced that the first check is not useless.
##########
File path: libs/libc/time/lib_localtime.c
##########
@@ -2528,9 +2537,20 @@ void tzset(void)
{
FAR const char *name;
- tz_semtake(&g_lcl_sem);
-
name = getenv("TZ");
+ if (g_lcl_isset > 0 && name && strcmp(g_lcl_tzname, name) == 0)
+ {
+ return;
+ }
Review comment:
My example was assuming both threads in user context, not in interrupt
context.
If I understood correctly, the thread executing in interrupt context will
just exit `tzset`, making again the first check redundant.
##########
File path: libs/libc/time/lib_localtime.c
##########
@@ -2528,9 +2537,20 @@ void tzset(void)
{
FAR const char *name;
- tz_semtake(&g_lcl_sem);
-
name = getenv("TZ");
+ if (g_lcl_isset > 0 && name && strcmp(g_lcl_tzname, name) == 0)
+ {
+ return;
+ }
Review comment:
My example was assuming both threads in user context, not in interrupt
context.
If I understood correctly, the thread executing in interrupt context will
just return from `tzset`, making again the first check redundant.
##########
File path: libs/libc/time/lib_localtime.c
##########
@@ -2528,9 +2537,20 @@ void tzset(void)
{
FAR const char *name;
- tz_semtake(&g_lcl_sem);
-
name = getenv("TZ");
+ if (g_lcl_isset > 0 && name && strcmp(g_lcl_tzname, name) == 0)
+ {
+ return;
+ }
+
+#ifndef __KERNEL__
+ if (up_interrupt_context())
+ {
+ return;
+ }
Review comment:
The point here is responsibility, which I think is important for having
a well-defined interface.
Maybe we should add to time-related APIs that they should not be called from
interrupt context.
Don't you think that, although an easier change, checking `if
(up_interrupt_context())` here seems more like a quick fix?
For me it makes more sense to explicitly state the usage restriction on the
APIs and fix the API users accordingly.
##########
File path: libs/libc/time/lib_localtime.c
##########
@@ -2528,9 +2537,20 @@ void tzset(void)
{
FAR const char *name;
- tz_semtake(&g_lcl_sem);
-
name = getenv("TZ");
+ if (g_lcl_isset > 0 && name && strcmp(g_lcl_tzname, name) == 0)
+ {
+ return;
+ }
+
+#ifndef __KERNEL__
+ if (up_interrupt_context())
+ {
+ return;
+ }
Review comment:
Not being able to be called from interrupt context is a real limitation
for time-related APIs.
There are similar examples throughout NuttX code where assumption and
limitations are documented on function APIs:
https://github.com/apache/incubator-nuttx/blob/master/net/arp/arp.h#L253-L255
##########
File path: libs/libc/time/lib_localtime.c
##########
@@ -2528,9 +2537,20 @@ void tzset(void)
{
FAR const char *name;
- tz_semtake(&g_lcl_sem);
-
name = getenv("TZ");
+ if (g_lcl_isset > 0 && name && strcmp(g_lcl_tzname, name) == 0)
+ {
+ return;
+ }
+
+#ifndef __KERNEL__
+ if (up_interrupt_context())
+ {
+ return;
+ }
Review comment:
> If we allow kernel component call locale time related API, the check
need be done here instead each call site.
I think I got your point. This restriction does not apply to drivers since
this change only targets the non-kernel build.
Indeed, it would be a bit hard to check in each call site.
##########
File path: libs/libc/time/lib_localtime.c
##########
@@ -2528,9 +2537,20 @@ void tzset(void)
{
FAR const char *name;
- tz_semtake(&g_lcl_sem);
-
name = getenv("TZ");
+ if (g_lcl_isset > 0 && name && strcmp(g_lcl_tzname, name) == 0)
+ {
+ return;
+ }
Review comment:
Honestly, I don't know.
The point I am having trouble to understand is why is the first check
needed, this one:
```c
if (g_lcl_isset > 0 && name && strcmp(g_lcl_tzname, name) == 0)
{
return;
}
```
In my mind right now there is the following question: Why isn't the fix just
the addition of the following check in the function entry:
```c
#ifndef __KERNEL__
if (up_interrupt_context())
{
return;
}
#endif
```
Because the code paths I am considering are:
1) If it's executing in interrupt context, the function will return either
by the first or by the second condition.
2) If it's executing in user/process context, it will check the global
variables twice, first without locking the semaphore (not thread-safe) and
second by locking the semaphore (thread-safe).
##########
File path: libs/libc/time/lib_localtime.c
##########
@@ -2528,9 +2537,20 @@ void tzset(void)
{
FAR const char *name;
- tz_semtake(&g_lcl_sem);
-
name = getenv("TZ");
+ if (g_lcl_isset > 0 && name && strcmp(g_lcl_tzname, name) == 0)
+ {
+ return;
+ }
Review comment:
Honestly, I don't know.
The point I am having trouble to understand is why is the first check
needed, this one:
```c
if (g_lcl_isset > 0 && name && strcmp(g_lcl_tzname, name) == 0)
{
return;
}
```
In my mind right now there is the following question: Why isn't the fix just
the addition of the following check in the function entry:
```c
#ifndef __KERNEL__
if (up_interrupt_context())
{
return;
}
#endif
```
Because the code paths I am considering are:
1) If it's executing in interrupt context, the function will return either
by the first or by the second condition.
2) If it's executing in user/process context, it will check the global
variables twice, first without locking the semaphore (not thread-safe) and
second by locking the semaphore (thread-safe).
So, why is that unsafe check required at the function entry?
##########
File path: libs/libc/time/lib_localtime.c
##########
@@ -2528,9 +2537,20 @@ void tzset(void)
{
FAR const char *name;
- tz_semtake(&g_lcl_sem);
-
name = getenv("TZ");
+ if (g_lcl_isset > 0 && name && strcmp(g_lcl_tzname, name) == 0)
+ {
+ return;
+ }
Review comment:
Honestly, I don't know.
The point I am having trouble to understand is why is the first check
needed, this one:
```c
if (g_lcl_isset > 0 && name && strcmp(g_lcl_tzname, name) == 0)
{
return;
}
```
In my mind right now there is the following question: Why isn't the fix just
the addition of the following check in the function entry:
```c
#ifndef __KERNEL__
if (up_interrupt_context())
{
return;
}
#endif
```
Because the code paths I am considering are:
1) If it's executing in interrupt context, the function will return either
by the first or by the second condition.
2) If it's executing in user/process context, it will check the global
variables twice, first without locking the semaphore (not thread-safe) and
second by locking the semaphore (thread-safe).
So, why is that unsafe check required at the function entry?
You confirmed before that is not for optimization purposes. So, for what
purpose is this?
##########
File path: libs/libc/time/lib_localtime.c
##########
@@ -2528,9 +2537,20 @@ void tzset(void)
{
FAR const char *name;
- tz_semtake(&g_lcl_sem);
-
name = getenv("TZ");
+ if (g_lcl_isset > 0 && name && strcmp(g_lcl_tzname, name) == 0)
+ {
+ return;
+ }
Review comment:
I think I've finally understood why you added the first check.
But only after I compared to the fix applied to `gmtsub`. There the same
"redundant" check has been applied, but on `gmtsub` is not really redundant.
On `gmtsub` the global variable `g_gmt_isset` is verified because, even if
executing in interrupt context, the function is able to perform the subtraction
because the timezone is already initialized.
But on `tzset` the verification is indeed redundant, for the reasons I've
explained before. When executing in interrupt context, `tzset` will just return
early without returning anything, regardless of whether the timezone is already
initialized or not.
Is this right?
##########
File path: libs/libc/time/lib_localtime.c
##########
@@ -2528,9 +2537,20 @@ void tzset(void)
{
FAR const char *name;
- tz_semtake(&g_lcl_sem);
-
name = getenv("TZ");
+ if (g_lcl_isset > 0 && name && strcmp(g_lcl_tzname, name) == 0)
+ {
+ return;
+ }
Review comment:
I think I've finally understood why you added the first check. But only
after I compared to the fix applied to `gmtsub`.
There, the same "redundant" check has been applied, but on `gmtsub` is not
really redundant.
On `gmtsub` the global variable `g_gmt_isset` is verified because, even if
executing in interrupt context, the function is able to perform the subtraction
because the timezone is already initialized.
But on `tzset` the verification is indeed redundant, for the reasons I've
explained before. When executing in interrupt context, `tzset` will just return
early without returning anything, regardless of whether the timezone is already
initialized or not.
Is this right?
##########
File path: libs/libc/time/lib_localtime.c
##########
@@ -2528,9 +2537,20 @@ void tzset(void)
{
FAR const char *name;
- tz_semtake(&g_lcl_sem);
-
name = getenv("TZ");
+ if (g_lcl_isset > 0 && name && strcmp(g_lcl_tzname, name) == 0)
+ {
+ return;
+ }
Review comment:
I think I've finally understood why you added the first check. But only
after I compared to the fix applied to `gmtsub`.
There, the same "redundant" check has been applied, but on `gmtsub` is not
really redundant.
On `gmtsub` the global variable `g_gmt_isset` is verified because, even if
executing in interrupt context, the function is able to perform the subtraction
because the timezone is already initialized.
But on `tzset` the verification is indeed redundant, for the reasons I've
explained before. When executing in interrupt context, `tzset` will just return
early without returning anything, regardless of whether the timezone is already
initialized or not. So it could be safely removed.
Is this right?
##########
File path: libs/libc/time/lib_localtime.c
##########
@@ -2528,9 +2537,20 @@ void tzset(void)
{
FAR const char *name;
- tz_semtake(&g_lcl_sem);
-
name = getenv("TZ");
+ if (g_lcl_isset > 0 && name && strcmp(g_lcl_tzname, name) == 0)
+ {
+ return;
+ }
Review comment:
I think I've finally understood why you added the first check. But only
after I compared to the fix applied to `gmtsub`.
There, the same "redundant" check has been applied, but on `gmtsub` is not
really redundant.
On `gmtsub` the global variable `g_gmt_isset` is verified because, even if
executing in interrupt context, the function is able to perform the subtraction
because the timezone is already initialized.
But on `tzset` the verification is indeed redundant, for the reasons I've
explained before. When executing in interrupt context, `tzset` will just return
early without returning anything, regardless of whether the timezone is already
initialized or not. So it could be safely removed.
Is this correct?
##########
File path: libs/libc/time/lib_localtime.c
##########
@@ -2528,9 +2537,20 @@ void tzset(void)
{
FAR const char *name;
- tz_semtake(&g_lcl_sem);
-
name = getenv("TZ");
+ if (g_lcl_isset > 0 && name && strcmp(g_lcl_tzname, name) == 0)
+ {
+ return;
+ }
Review comment:
I think I've finally understood why you added the first check. But only
after I compared to the fix applied to `gmtsub`.
There, the same "redundant" check has been applied, but on `gmtsub` is not
really redundant.
On `gmtsub` the global variable `g_gmt_isset` is verified because, even if
executing in interrupt context, the function is able to perform the subtraction
because the timezone is already initialized.
But on `tzset` the verification is indeed redundant, for the reasons I've
explained before. When executing in interrupt context, `tzset` will just return
early without returning anything, regardless of whether the timezone is already
initialized or not. So the first check could be safely removed.
Is this correct?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
