Re: [Qemu-devel] [PATCH] ivshmem-server: Detect and use if there is required -lrt linking

[Markus Armbruster]

Kamil Rytarowski <address@hidden> writes:

> On 22.05.2017 08:28, Markus Armbruster wrote:
>> Kamil Rytarowski <address@hidden> writes:
>> 
>>> Hello,
>>>
>>> Excuse me for delay, I missed this mail.
>> 
>> No problem.
>> 
>>> Please see in-line.
>>>
>>> On 17.05.2017 09:28, Markus Armbruster wrote:
>>>> Kamil Rytarowski <address@hidden> writes:
>>>>
>>>>> ivshmem-server makes use of the POSIX shared memory object interfaces.
>>>>> This library is provided on NetBSD in -lrt (POSIX Real-time Library).
>>>>> Add ./configure check if there is needed -lrt linking for shm_open()
>>>>> and if so use it. Introduce new configure generated variable LIBS_SHMLIB.
>>>>>
>>>>> This fixes build issue on NetBSD.
>>>>>
>>>>> Signed-off-by: Kamil Rytarowski <address@hidden>
>>>>> ---
>>>>>  Makefile  |  1 +
>>>>>  configure | 20 ++++++++++++++++++++
>>>>>  2 files changed, 21 insertions(+)
>>>>>
>>>>> diff --git a/Makefile b/Makefile
>>>>> index 31d41a7eae..3248cb53d7 100644
>>>>> --- a/Makefile
>>>>> +++ b/Makefile
>>>>> @@ -473,6 +473,7 @@ ivshmem-client$(EXESUF): $(ivshmem-client-obj-y) 
>>>>> $(COMMON_LDADDS)
>>>>>   $(call LINK, $^)
>>>>>  ivshmem-server$(EXESUF): $(ivshmem-server-obj-y) $(COMMON_LDADDS)
>>>>>   $(call LINK, $^)
>>>>> +ivshmem-server$(EXESUF): LIBS += $(LIBS_SHMLIB)
>>>>>  
>>>>>  module_block.h: $(SRC_PATH)/scripts/modules/module_block.py 
>>>>> config-host.mak
>>>>>   $(call quiet-command,$(PYTHON) $< $@ \
>>>>> diff --git a/configure b/configure
>>>>> index 7c020c076b..50c3aee746 100755
>>>>> --- a/configure
>>>>> +++ b/configure
>>>>> @@ -179,6 +179,7 @@ audio_pt_int=""
>>>>>  audio_win_int=""
>>>>>  cc_i386=i386-pc-linux-gnu-gcc
>>>>>  libs_qga=""
>>>>> +libs_shmlib=""
>>>>>  debug_info="yes"
>>>>>  stack_protector=""
>>>>>  
>>>>> @@ -4133,6 +4134,24 @@ elif compile_prog "" "$pthread_lib -lrt" ; then
>>>>>    libs_qga="$libs_qga -lrt"
>>>>>  fi
>>>>>  
>>>>> +##########################################
>>>>> +# Do we need librt for shm_open()
>>>>> +cat > $TMPC <<EOF
>>>>> +#include <sys/mman.h>
>>>>> +#include <sys/stat.h>
>>>>> +#include <fcntl.h>
>>>>> +#include <stddef.h>
>>>>> +int main(void) {
>>>>> +  return shm_open(NULL, O_RDWR, 0644);
>>>>> +}
>>>>> +EOF
>>>>> +
>>>>> +if compile_prog "" "" ; then
>>>>> +  :
>>>>> +elif compile_prog "" "-lrt" ; then
>>>>> +  libs_shmlib="$libs_shmlib -lrt"
>>>>> +fi
>>>>> +
>>>>>  if test "$darwin" != "yes" -a "$mingw32" != "yes" -a "$solaris" != yes 
>>>>> -a \
>>>>>          "$aix" != "yes" -a "$haiku" != "yes" ; then
>>>>>      libs_softmmu="-lutil $libs_softmmu"
>>>>> @@ -5949,6 +5968,7 @@ echo "EXESUF=$EXESUF" >> $config_host_mak
>>>>>  echo "DSOSUF=$DSOSUF" >> $config_host_mak
>>>>>  echo "LDFLAGS_SHARED=$LDFLAGS_SHARED" >> $config_host_mak
>>>>>  echo "LIBS_QGA+=$libs_qga" >> $config_host_mak
>>>>> +echo "LIBS_SHMLIB+=$libs_shmlib" >> $config_host_mak
>>>>>  echo "TASN1_LIBS=$tasn1_libs" >> $config_host_mak
>>>>>  echo "TASN1_CFLAGS=$tasn1_cflags" >> $config_host_mak
>>>>>  echo "POD2MAN=$POD2MAN" >> $config_host_mak
>>>>
>>>> We already have a test for -lrt.
>>>
>>> Correct.
>>>
>>>>  It looks for timer_create() and
>>>> clock_gettime().
>>>
>>>
>>> timer_create(2) and clock_settime(2) are in libc on NetBSD.
>>>
>>>>  If we need -lrt,
>>>
>>> We need it just for shm_open(3).
>>>
>>>> we add it to LIBS and to LIBS_QGA.
>>>> The latter because we don't use LIBS for qemu-ga:
>>>>
>>>>     qemu-ga$(EXESUF): LIBS = $(LIBS_QGA)
>>>>
>>>> This patch adds a second test for -lrt, to look for shm_open().  It adds
>>>> -lrt to new variable LIBS_SHMLIB, which gets used only for
>>>> ivshmem-server:
>>>>
>>>>     ivshmem-server$(EXESUF): LIBS += $(LIBS_SHMLIB)
>>>>
>>>> Note that ivshmem-server already uses LIBS.
>>>>
>>>> Shouldn't we instead widen the existing test for -lrt to cover
>>>> shm_open()?
>>>>
>>>
>>> I will prepare patch in the requested way. I don't have preference.
>>>
>>>> Can you confirm that the existing test does not add -lrt to LIBS on
>>>> NetBSD?
>>>>
>>>
>>> config-host.mak:LIBS+=-lm -L/usr/pkg/lib -lgthread-2.0 -pthread
>>> -Wl,-R/usr/pkg/lib -lglib-2.0 -lintl   -lz
>>> config-host.mak:LIBS_QGA+=-lm -L/usr/pkg/lib -lgthread-2.0 -pthread
>>> -Wl,-R/usr/pkg/lib -lglib-2.0 -lintl
>>>
>>>> tests/ivshmem-test.c also calls shm_open().  Does it work on NetBSD?
>>>>
>>>
>>> Currently it's disabled, as it requires eventfd() (Linux API).
>> 
>> You're right.
>> 
>> So is the ivshmem device.  Hmm.  Why would anyone want ivshmem-server on
>> NetBSD?  Its purpose is connecting ivshmem-doorbell devices.  Could we
>> simply add the obvious CONFIG_IVSHMEM dependence to
>> contrib/ivshmem-*/Makefile.objs?
>> 
>
> In general I would like to get feature parity on NetBSD, there is no
> reason to blacklist this feature for !Linux hosts. However short term

Feature parity is a worthy goal, but compiling ivshmem-server without
ivshmem-doorbell doesn't gets you a feature, it gets you a binary you
can't use for its intended purpose :)

> there are higher priorities, to fix build of pristine sources
> (ivshmem-server is blocking here, not that I'm right now working on its
> correctness on the NetBSD host) and run test suite.

Your priorities make sense.

> I wanted to avoid overlinking, that would be caused by adding -lrt.
> Adding dedicated -lrt check for shm_open(3) made sense to me, but I will
> go the requested route.

Avoiding overlinking makes sense, too.