[1/2] compat/win32/pthread: add pthread_once()
diff mbox series

Message ID e5a10d3f2152859b75bd815c37511975057d0ab0.1593115455.git.matheus.bernardino@usp.br
State New
Headers show
Series
  • Make oid_to_hex() thread-safe
Related show

Commit Message

Matheus Tavares Bernardino June 25, 2020, 8:32 p.m. UTC
Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
---

Note: the pthread_once() function is adapted from:
https://git.libav.org/?p=libav.git;a=commitdiff;h=b22693b06d1e5d73454a65c203b4d31c1ca5b69a

Which is LGPLv2.1. Should I add any notice/acknowledgment somewhere,
besides the comment I added above the function?

 compat/win32/pthread.c | 22 ++++++++++++++++++++++
 compat/win32/pthread.h |  5 +++++
 thread-utils.c         | 11 +++++++++++
 thread-utils.h         |  6 ++++++
 4 files changed, 44 insertions(+)

Comments

Christian Couder June 26, 2020, 5:45 a.m. UTC | #1
On Fri, Jun 26, 2020 at 3:00 AM Matheus Tavares
<matheus.bernardino@usp.br> wrote:
>
> Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
> ---

The commit message might want to explain a bit the purpose of adding
these features.

> Note: the pthread_once() function is adapted from:
> https://git.libav.org/?p=libav.git;a=commitdiff;h=b22693b06d1e5d73454a65c203b4d31c1ca5b69a
>
> Which is LGPLv2.1. Should I add any notice/acknowledgment somewhere,
> besides the comment I added above the function?

Yeah, I think you should also tell in the commit message where the
code comes from (along with the hash of the commit) and that libav is
LGPLv2.1 which is compatible with GPLv2 as explained in section 3 of
the LGPLv2.1.

>  compat/win32/pthread.c | 22 ++++++++++++++++++++++
>  compat/win32/pthread.h |  5 +++++
>  thread-utils.c         | 11 +++++++++++
>  thread-utils.h         |  6 ++++++
>  4 files changed, 44 insertions(+)
>
> diff --git a/compat/win32/pthread.c b/compat/win32/pthread.c
> index 2e7eead42c..5a7ecbd999 100644
> --- a/compat/win32/pthread.c
> +++ b/compat/win32/pthread.c
> @@ -56,3 +56,25 @@ pthread_t pthread_self(void)
>         t.tid = GetCurrentThreadId();
>         return t;
>  }
> +
> +/* Adapted from libav's compat/w32pthreads.h. */
> +int pthread_once(pthread_once_t *once_control, void (*init_routine)(void))
> +{
> +       BOOL pending = FALSE;
> +       int ret = 0;
> +
> +       if(!InitOnceBeginInitialize(once_control, 0, &pending, NULL)) {

We put a space between "if" and the following "(". It might also be
interesting to know perhaps in the commit message how much you adapted
the code.

For example perhaps a good strategy would be in the commit that
imports the code to do the minimal amount of change so that it builds
and passes the test, and then to have another commit that adapts the
style of the code.

> +               ret = err_win_to_posix(GetLastError());
> +               goto out;
> +       }
> +
> +       if (pending)
> +               init_routine();
> +
> +       if(!InitOnceComplete(once_control, 0, NULL))

Space missing between "if" and the following "(".

> +               ret = err_win_to_posix(GetLastError());
> +
> +out:
> +       /* POSIX doesn't allow pthread_once() to return EINTR */
> +       return ret == EINTR ? EIO : ret;
> +}
Matheus Tavares Bernardino June 26, 2020, 2:13 p.m. UTC | #2
On Fri, Jun 26, 2020 at 2:45 AM Christian Couder
<christian.couder@gmail.com> wrote:
>
> On Fri, Jun 26, 2020 at 3:00 AM Matheus Tavares
> <matheus.bernardino@usp.br> wrote:
> >
> > Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
> > ---
>
> The commit message might want to explain a bit the purpose of adding
> these features.

Will do!

> > Note: the pthread_once() function is adapted from:
> > https://git.libav.org/?p=libav.git;a=commitdiff;h=b22693b06d1e5d73454a65c203b4d31c1ca5b69a
> >
> > Which is LGPLv2.1. Should I add any notice/acknowledgment somewhere,
> > besides the comment I added above the function?
>
> Yeah, I think you should also tell in the commit message where the
> code comes from (along with the hash of the commit) and that libav is
> LGPLv2.1 which is compatible with GPLv2 as explained in section 3 of
> the LGPLv2.1.

Sounds good, I'll add that information.

> >  compat/win32/pthread.c | 22 ++++++++++++++++++++++
> >  compat/win32/pthread.h |  5 +++++
> >  thread-utils.c         | 11 +++++++++++
> >  thread-utils.h         |  6 ++++++
> >  4 files changed, 44 insertions(+)
> >
> > diff --git a/compat/win32/pthread.c b/compat/win32/pthread.c
> > index 2e7eead42c..5a7ecbd999 100644
> > --- a/compat/win32/pthread.c
> > +++ b/compat/win32/pthread.c
> > @@ -56,3 +56,25 @@ pthread_t pthread_self(void)
> >         t.tid = GetCurrentThreadId();
> >         return t;
> >  }
> > +
> > +/* Adapted from libav's compat/w32pthreads.h. */
> > +int pthread_once(pthread_once_t *once_control, void (*init_routine)(void))
> > +{
> > +       BOOL pending = FALSE;
> > +       int ret = 0;
> > +
> > +       if(!InitOnceBeginInitialize(once_control, 0, &pending, NULL)) {
>
> We put a space between "if" and the following "(". It might also be
> interesting to know perhaps in the commit message how much you adapted
> the code.
>
> For example perhaps a good strategy would be in the commit that
> imports the code to do the minimal amount of change so that it builds
> and passes the test, and then to have another commit that adapts the
> style of the code.

Makes sense. Alternatively, since the code ported from libav is quite
small, I think we might as well already adapt the style in this same
commit. I'll fix that for v2, thanks!

Patch
diff mbox series

diff --git a/compat/win32/pthread.c b/compat/win32/pthread.c
index 2e7eead42c..5a7ecbd999 100644
--- a/compat/win32/pthread.c
+++ b/compat/win32/pthread.c
@@ -56,3 +56,25 @@  pthread_t pthread_self(void)
 	t.tid = GetCurrentThreadId();
 	return t;
 }
+
+/* Adapted from libav's compat/w32pthreads.h. */
+int pthread_once(pthread_once_t *once_control, void (*init_routine)(void))
+{
+	BOOL pending = FALSE;
+	int ret = 0;
+
+	if(!InitOnceBeginInitialize(once_control, 0, &pending, NULL)) {
+		ret = err_win_to_posix(GetLastError());
+		goto out;
+	}
+
+	if (pending)
+		init_routine();
+
+	if(!InitOnceComplete(once_control, 0, NULL))
+		ret = err_win_to_posix(GetLastError());
+
+out:
+	/* POSIX doesn't allow pthread_once() to return EINTR */
+	return ret == EINTR ? EIO : ret;
+}
diff --git a/compat/win32/pthread.h b/compat/win32/pthread.h
index 737983d00b..c50f1e89c7 100644
--- a/compat/win32/pthread.h
+++ b/compat/win32/pthread.h
@@ -40,6 +40,11 @@  typedef int pthread_mutexattr_t;
 #define pthread_cond_signal WakeConditionVariable
 #define pthread_cond_broadcast WakeAllConditionVariable
 
+#define pthread_once_t INIT_ONCE
+
+#define PTHREAD_ONCE_INIT INIT_ONCE_STATIC_INIT
+int pthread_once(pthread_once_t *once_control, void (*init_routine)(void));
+
 /*
  * Simple thread creation implementation using pthread API
  */
diff --git a/thread-utils.c b/thread-utils.c
index 5329845691..937deb3f2e 100644
--- a/thread-utils.c
+++ b/thread-utils.c
@@ -122,4 +122,15 @@  int dummy_pthread_join(pthread_t pthread, void **retval)
 	return ENOSYS;
 }
 
+int nothreads_pthread_once(pthread_once_t *once_control,
+			   void (*init_routine)(void))
+{
+	if (*once_control == 1)
+		return 0;
+
+	init_routine();
+	*once_control = 1;
+	return 0;
+}
+
 #endif
diff --git a/thread-utils.h b/thread-utils.h
index 4961487ed9..bab9dc5e4d 100644
--- a/thread-utils.h
+++ b/thread-utils.h
@@ -19,6 +19,7 @@ 
 #define pthread_mutex_t int
 #define pthread_cond_t int
 #define pthread_key_t int
+#define pthread_once_t int
 
 #define pthread_mutex_init(mutex, attr) dummy_pthread_init(mutex)
 #define pthread_mutex_lock(mutex)
@@ -48,6 +49,11 @@  int dummy_pthread_join(pthread_t pthread, void **retval);
 
 int dummy_pthread_init(void *);
 
+#define PTHREAD_ONCE_INIT 0
+int nothreads_pthread_once(pthread_once_t *once_control,
+			   void (*init_routine)(void));
+#define pthread_once(once, routine) nothreads_pthread_once(once, routine)
+
 #endif
 
 int online_cpus(void);