Re: [PATCH-for-5.2 v3] configure: Check vhost-user is available for vhost-user-blk-server

[Philippe Mathieu-Daudé]

On 11/9/20 7:52 PM, Philippe Mathieu-Daudé wrote:
> On 11/9/20 7:44 PM, Thomas Huth wrote:
>> On 09/11/2020 19.41, Philippe Mathieu-Daudé wrote:
>>> On Mon, Nov 9, 2020 at 6:22 PM Thomas Huth <thuth@redhat.com> wrote:
>>>> On 09/11/2020 15.07, Philippe Mathieu-Daudé wrote:
>>>>> Check vhost-user is available when building vhost-user-blk-server.
>>>>>
>>>>> This fixes:
>>>>>
>>>>>  $ ../configure \
>>>>>       --disable-vhost-user --enable-vhost-user-blk-server && \
>>>>>    make qemu-nbd
>>>>>  ...
>>>>>  [505/505] Linking target qemu-nbd
>>>>>  FAILED: qemu-nbd
>>>>>  cc  -o qemu-nbd qemu-nbd.p/qemu-nbd.c.o -Wl,--as-needed 
>>>>> -Wl,--no-undefined -pie -Wl,--whole-archive libblockdev.fa libblock.fa 
>>>>> libcrypto.fa libauthz.fa libqom.fa libio.fa -Wl,--no-whole-archive 
>>>>> -Wl,--warn-common -Wl,-z,relro -Wl,-z,now -m64 -fstack-protector-strong 
>>>>> -Wl,--start-group libqemuutil.a libblockdev.fa libblock.fa libcrypto.fa 
>>>>> libauthz.fa libqom.fa libio.fa @block.syms -lgio-2.0 -lgobject-2.0 
>>>>> -lglib-2.0 -lgio-2.0 -lgobject-2.0 -lglib-2.0 -pthread -lgnutls -lutil 
>>>>> -lm -lgthread-2.0 -lglib-2.0 -lbz2 -lgthread-2.0 -lglib-2.0 -lssh -lrbd 
>>>>> -lrados -lcurl -lxml2 -lzstd -lacl -lgfapi -lglusterfs -lgfrpc -lgfxdr 
>>>>> -luuid -laio /usr/lib64/libz.so -L/usr/lib64/iscsi -liscsi -lnettle 
>>>>> -lgnutls -lpam -Wl,--end-group
>>>>>  /usr/bin/ld: libblockdev.fa(block_export_vhost-user-blk-server.c.o): in 
>>>>> function `vu_blk_process_vq':
>>>>>  block/export/vhost-user-blk-server.c:203: undefined reference to 
>>>>> `vu_get_queue'
>>>>>  /usr/bin/ld: block/export/vhost-user-blk-server.c:208: undefined 
>>>>> reference to `vu_queue_pop'
>>>>>  /usr/bin/ld: libblockdev.fa(block_export_vhost-user-blk-server.c.o): in 
>>>>> function `vu_blk_queue_set_started':
>>>>>  block/export/vhost-user-blk-server.c:228: undefined reference to 
>>>>> `vu_get_queue'
>>>>>  /usr/bin/ld: libblockdev.fa(block_export_vhost-user-blk-server.c.o): in 
>>>>> function `vu_blk_req_complete':
>>>>>  block/export/vhost-user-blk-server.c:55: undefined reference to 
>>>>> `vu_queue_push'
>>>>>  /usr/bin/ld: block/export/vhost-user-blk-server.c:56: undefined 
>>>>> reference to `vu_queue_notify'
>>>>>  /usr/bin/ld: libblockdev.fa(block_export_vhost-user-blk-server.c.o): in 
>>>>> function `vu_blk_queue_set_started':
>>>>>  block/export/vhost-user-blk-server.c:229: undefined reference to 
>>>>> `vu_set_queue_handler'
>>>>>  /usr/bin/ld: libqemuutil.a(util_vhost-user-server.c.o): in function 
>>>>> `vu_client_trip':
>>>>>  util/vhost-user-server.c:176: undefined reference to `vu_dispatch'
>>>>>  /usr/bin/ld: util/vhost-user-server.c:180: undefined reference to 
>>>>> `vu_deinit'
>>>>>  /usr/bin/ld: libqemuutil.a(util_vhost-user-server.c.o): in function 
>>>>> `vu_accept':
>>>>>  util/vhost-user-server.c:291: undefined reference to `vu_init'
>>>>>  collect2: error: ld returned 1 exit status
>>>>>  ninja: build stopped: subcommand failed.
>>>>>  make: *** [Makefile:171: run-ninja] Error 1
>>>>>
>>>>> Now we get:
>>>>>
>>>>>  $ ../configure \
>>>>>       --disable-vhost-user --enable-vhost-user-blk-server && \
>>>>>  ERROR: --enable-vhost-user-blk-server requires --enable-vhost-user
>>>>>
>>>>> Fixes: bc15e44cb21 ("configure: introduce --enable-vhost-user-blk-server")
>>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>>>> ---
>>>>> Since v1:
>>>>> - Addressed Thomas review comments
>>>>> https://lists.gnu.org/archive/html/qemu-devel/2020-11/msg02188.html
>>>>> ---
>>>>>  configure | 4 ++++
>>>>>  1 file changed, 4 insertions(+)
>>>>>
>>>>> diff --git a/configure b/configure
>>>>> index 805f7791503..3124be82da1 100755
>>>>> --- a/configure
>>>>> +++ b/configure
>>>>> @@ -2375,6 +2375,10 @@ test "$vhost_user_fs" = "" && 
>>>>> vhost_user_fs=$vhost_user
>>>>>  if test "$vhost_user_fs" = "yes" && test "$vhost_user" = "no"; then
>>>>>    error_exit "--enable-vhost-user-fs requires --enable-vhost-user"
>>>>>  fi
>>>>> +test "$vhost_user_blk_server" = "" && vhost_user_blk_server=$vhost_user
>>>>> +if test "$vhost_user_blk_server" = "yes" && test "$vhost_user" = "no"; 
>>>>> then
>>>>> +  error_exit "--enable-vhost-user-blk-server requires 
>>>>> --enable-vhost-user"
>>>>> +fi
>>>>
>>>> A little bit later in this file, we've got :
>>>>
>>>> # libvhost-user is Linux-only
>>>> test "$vhost_user_blk_server" = "" && vhost_user_blk_server=$linux
>>>> if test "$vhost_user_blk_server" = "yes" && test "$linux" = "no"; then
>>>>   error_exit "--enable-vhost-user-blk-server is only available on Linux"
>>>> fi
>>>>
>>>> I think it would be better to add the new code there instead.
>>>> Also maybe better do something like:
>>>>
>>>> test "$vhost_user_blk_server" = "" && test "$vhost_user" = "yes" \
>>>>  &&  vhost_user_blk_server=$linux
>>>>
>>>> instead?
>>>
>>> I understood this option is similar to the other vhost-user related ones
>>> and should be treated the same way.
>>
>> But the second check does not make much sense anymore after your change,
>> since "$vhost_user_blk_server" is certainly not empty anymore. So the two
>> checks IMHO have to be combined.
> 
> Ah now I see it... You are right, thanks for the review :)

So we are back to v2:
https://lists.gnu.org/archive/html/qemu-devel/2020-11/msg02333.html