1. Patchew
  2. Xen
  3. View series

[PATCH] tools/libs/store: add missing USE_PTHREAD to target .o

Andrei Stan posted 1 patch 2 months ago
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20240705145910.32736-3-andreistan2003@gmail.com
tools/libs/store/Makefile | 1 +
1 file changed, 1 insertion(+)
[PATCH] tools/libs/store: add missing USE_PTHREAD to target .o
Posted by Andrei Stan 2 months ago 
Currently only shared libraries build with USE_PTHREAD enabled.

This patch adds the macro also to xs.o.

Backport: 4.18+ # maybe older
Signed-off-by: Andrei Stan <andreistan2003@gmail.com>
---
 tools/libs/store/Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/libs/store/Makefile b/tools/libs/store/Makefile
index 0649cf8307..c6f147c72f 100644
--- a/tools/libs/store/Makefile
+++ b/tools/libs/store/Makefile
@@ -20,6 +20,7 @@ CFLAGS += -include $(XEN_ROOT)/tools/config.h
 CFLAGS += $(CFLAGS_libxentoolcore)
 
 xs.opic: CFLAGS += -DUSE_PTHREAD
+xs.o: CFLAGS += -DUSE_PTHREAD
 ifeq ($(CONFIG_Linux),y)
 xs.opic: CFLAGS += -DUSE_DLSYM
 endif
-- 
2.43.0
Re: [PATCH] tools/libs/store: add missing USE_PTHREAD to target .o
Posted by Jürgen Groß 2 months ago 
On 05.07.24 16:59, Andrei Stan wrote:
> Currently only shared libraries build with USE_PTHREAD enabled.
> 
> This patch adds the macro also to xs.o.
> 
> Backport: 4.18+ # maybe older
> Signed-off-by: Andrei Stan <andreistan2003@gmail.com>

Commit dde3e02b7068 did that explicitly, stating that phtreads are
not compatible with static linking.

Was that reasoning wrong?


Juergen

> ---
>   tools/libs/store/Makefile | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/tools/libs/store/Makefile b/tools/libs/store/Makefile
> index 0649cf8307..c6f147c72f 100644
> --- a/tools/libs/store/Makefile
> +++ b/tools/libs/store/Makefile
> @@ -20,6 +20,7 @@ CFLAGS += -include $(XEN_ROOT)/tools/config.h
>   CFLAGS += $(CFLAGS_libxentoolcore)
>   
>   xs.opic: CFLAGS += -DUSE_PTHREAD
> +xs.o: CFLAGS += -DUSE_PTHREAD
>   ifeq ($(CONFIG_Linux),y)
>   xs.opic: CFLAGS += -DUSE_DLSYM
>   endif
Re: [PATCH] tools/libs/store: add missing USE_PTHREAD to target .o
Posted by Andrei Stan 2 months ago 
On Fri, Jul 5, 2024 at 6:22 PM Jürgen Groß <jgross@suse.com> wrote:
>
> On 05.07.24 16:59, Andrei Stan wrote:
> > Currently only shared libraries build with USE_PTHREAD enabled.
> >
> > This patch adds the macro also to xs.o.
> >
> > Backport: 4.18+ # maybe older
> > Signed-off-by: Andrei Stan <andreistan2003@gmail.com>
>
> Commit dde3e02b7068 did that explicitly, stating that phtreads are
> not compatible with static linking.
>
> Was that reasoning wrong?
>
> Juergen
> ---
> > ---
> >   tools/libs/store/Makefile | 1 +
> >   1 file changed, 1 insertion(+)
> >
> > diff --git a/tools/libs/store/Makefile b/tools/libs/store/Makefile
> > index 0649cf8307..c6f147c72f 100644
> > --- a/tools/libs/store/Makefile
> > +++ b/tools/libs/store/Makefile
> > @@ -20,6 +20,7 @@ CFLAGS += -include $(XEN_ROOT)/tools/config.h
> >   CFLAGS += $(CFLAGS_libxentoolcore)
> >
> >   xs.opic: CFLAGS += -DUSE_PTHREAD
> > +xs.o: CFLAGS += -DUSE_PTHREAD
> >   ifeq ($(CONFIG_Linux),y)
> >   xs.opic: CFLAGS += -DUSE_DLSYM
> >   endif

It was a pretty nasty situation, giving some context, this is for a Go based CLI
tool and some things in there are multithreaded, but i don't think
calling in the
bindings happens anywhere in parallel. Without this patch there would be
some sort of race conditions (or so I assume from the debugging i've done)
happening withing the xen/tools code, making it impossible to start a domain.

In this case there were no issues in linking pthreads statically. I was not even
aware of that being a possible issue. Is it really?

Also I am not too sure how much that code path is actually tested, probably the
majority of the testing is done with dynamic builds.

Andrei
Re: [PATCH] tools/libs/store: add missing USE_PTHREAD to target .o
Posted by Andrei Stan 1 week ago 
Ping

On Fri, Jul 5, 2024 at 7:05 PM Andrei Stan <andreistan2003@gmail.com> wrote:
>
> On Fri, Jul 5, 2024 at 6:22 PM Jürgen Groß <jgross@suse.com> wrote:
> >
> > On 05.07.24 16:59, Andrei Stan wrote:
> > > Currently only shared libraries build with USE_PTHREAD enabled.
> > >
> > > This patch adds the macro also to xs.o.
> > >
> > > Backport: 4.18+ # maybe older
> > > Signed-off-by: Andrei Stan <andreistan2003@gmail.com>
> >
> > Commit dde3e02b7068 did that explicitly, stating that phtreads are
> > not compatible with static linking.
> >
> > Was that reasoning wrong?
> >
> > Juergen
> > ---
> > > ---
> > >   tools/libs/store/Makefile | 1 +
> > >   1 file changed, 1 insertion(+)
> > >
> > > diff --git a/tools/libs/store/Makefile b/tools/libs/store/Makefile
> > > index 0649cf8307..c6f147c72f 100644
> > > --- a/tools/libs/store/Makefile
> > > +++ b/tools/libs/store/Makefile
> > > @@ -20,6 +20,7 @@ CFLAGS += -include $(XEN_ROOT)/tools/config.h
> > >   CFLAGS += $(CFLAGS_libxentoolcore)
> > >
> > >   xs.opic: CFLAGS += -DUSE_PTHREAD
> > > +xs.o: CFLAGS += -DUSE_PTHREAD
> > >   ifeq ($(CONFIG_Linux),y)
> > >   xs.opic: CFLAGS += -DUSE_DLSYM
> > >   endif
>
> It was a pretty nasty situation, giving some context, this is for a Go based CLI
> tool and some things in there are multithreaded, but i don't think
> calling in the
> bindings happens anywhere in parallel. Without this patch there would be
> some sort of race conditions (or so I assume from the debugging i've done)
> happening withing the xen/tools code, making it impossible to start a domain.
>
> In this case there were no issues in linking pthreads statically. I was not even
> aware of that being a possible issue. Is it really?
>
> Also I am not too sure how much that code path is actually tested, probably the
> majority of the testing is done with dynamic builds.
>
> Andrei
Re: [PATCH] tools/libs/store: add missing USE_PTHREAD to target .o
Posted by Jan Beulich 5 days, 15 hours ago 
On 31.08.2024 12:34, Andrei Stan wrote:
> Ping

This is pinging what exactly? From ...

> On Fri, Jul 5, 2024 at 7:05 PM Andrei Stan <andreistan2003@gmail.com> wrote:
>>
>> On Fri, Jul 5, 2024 at 6:22 PM Jürgen Groß <jgross@suse.com> wrote:
>>>
>>> On 05.07.24 16:59, Andrei Stan wrote:
>>>> Currently only shared libraries build with USE_PTHREAD enabled.
>>>>
>>>> This patch adds the macro also to xs.o.
>>>>
>>>> Backport: 4.18+ # maybe older
>>>> Signed-off-by: Andrei Stan <andreistan2003@gmail.com>
>>>
>>> Commit dde3e02b7068 did that explicitly, stating that phtreads are
>>> not compatible with static linking.
>>>
>>> Was that reasoning wrong?

... Jürgen's question it imo follows that ...

>>>> ---
>>>>   tools/libs/store/Makefile | 1 +
>>>>   1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/tools/libs/store/Makefile b/tools/libs/store/Makefile
>>>> index 0649cf8307..c6f147c72f 100644
>>>> --- a/tools/libs/store/Makefile
>>>> +++ b/tools/libs/store/Makefile
>>>> @@ -20,6 +20,7 @@ CFLAGS += -include $(XEN_ROOT)/tools/config.h
>>>>   CFLAGS += $(CFLAGS_libxentoolcore)
>>>>
>>>>   xs.opic: CFLAGS += -DUSE_PTHREAD
>>>> +xs.o: CFLAGS += -DUSE_PTHREAD
>>>>   ifeq ($(CONFIG_Linux),y)
>>>>   xs.opic: CFLAGS += -DUSE_DLSYM
>>>>   endif
>>
>> It was a pretty nasty situation, giving some context, this is for a Go based CLI
>> tool and some things in there are multithreaded, but i don't think
>> calling in the
>> bindings happens anywhere in parallel. Without this patch there would be
>> some sort of race conditions (or so I assume from the debugging i've done)
>> happening withing the xen/tools code, making it impossible to start a domain.
>>
>> In this case there were no issues in linking pthreads statically. I was not even
>> aware of that being a possible issue. Is it really?
>>
>> Also I am not too sure how much that code path is actually tested, probably the
>> majority of the testing is done with dynamic builds.

... this answer is insufficient, from two angles:
- You do in no way answer the question itself, as such an answer would clearly
  need to say something about the commit Jürgen pointed you at.
- It needs to be in the patch description, not just in a reply on list. IOW a
  re-submission is needed anyway.

Jan
Re: [PATCH for-4.19?] tools/libs/store: add missing USE_PTHREAD to target .o
Posted by Andrew Cooper 2 months ago 
On 05/07/2024 3:59 pm, Andrei Stan wrote:
> Currently only shared libraries build with USE_PTHREAD enabled.
>
> This patch adds the macro also to xs.o.
>
> Backport: 4.18+ # maybe older
> Signed-off-by: Andrei Stan <andreistan2003@gmail.com>
> ---
>  tools/libs/store/Makefile | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/tools/libs/store/Makefile b/tools/libs/store/Makefile
> index 0649cf8307..c6f147c72f 100644
> --- a/tools/libs/store/Makefile
> +++ b/tools/libs/store/Makefile
> @@ -20,6 +20,7 @@ CFLAGS += -include $(XEN_ROOT)/tools/config.h
>  CFLAGS += $(CFLAGS_libxentoolcore)
>  
>  xs.opic: CFLAGS += -DUSE_PTHREAD
> +xs.o: CFLAGS += -DUSE_PTHREAD
>  ifeq ($(CONFIG_Linux),y)
>  xs.opic: CFLAGS += -DUSE_DLSYM
>  endif

Funnily enough, I did wonder whether that mattered recently.  I guess it
does.

CC Oleksii for a view to 4.19.

Also, we should transform the Backport: tag into a Fixes: tag if there's
a suitable one to pick.

~Andrew

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.