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

[Kamil Rytarowski]

On 23.05.2017 07:37, Markus Armbruster wrote:
> 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 :)
> 

$ qemu-system-x86_64 -device ? 2>&1 |grep ivsh
name "ivshmem", bus PCI, desc "Inter-VM shared memory (legacy)"
name "ivshmem-doorbell", bus PCI, desc "Inter-VM shared memory"
name "ivshmem-plain", bus PCI, desc "Inter-VM shared memory"

This will need definitely more work. We can disable ivshmem and hide the
problem, but it will be unveiled in future again and we will be back to
this discussion.

Also people can use NetBSD libc on Linux and share the same issue.

>> 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.
>

signature.asc
Description: OpenPGP digital signature