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

[Markus Armbruster]

Kamil Rytarowski <address@hidden> writes:

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

CONFIG_IVSHMEM defaults to CONFIG_EVENTFD.  If you compile with a
toolchain that doesn't provide eventfd(), configure should deselect
CONFIG_EVENTFD and thus CONFIG_IVSHMEM automatically.

Having contrib/ivshmem* depend on CONFIG_IVSHMEM makes total sense.
Looking forward to your patch.

[...]