Re: [PATCH v1 5/6] tests: only run ipmi-bt-test if CONFIG_LINUX

[Alex Bennée]

Corey Minyard <address@hidden> writes:

> On Mon, Nov 04, 2019 at 05:36:53PM +0000, Alex Bennée wrote:
>> This test has been unstable on NetBSD for awhile. It seems the
>> mechanism used to listen to a random port is a Linux-ism (although a
>> received wisdom Linux-ism rather than a well documented one). As
>
> Hmm.  I got reports of this issue and tried to reproduce, but I was
> never able to.  I thought I had fixed it via other means, but I had no
> idea this was the cause of the issue.
>
> It's not a Linux-ism, I don't think, Richard Steven's "Unix Network
> Programming" mentions that is how this works (at least on UDP), and that
> is pre-Linux.  That's probably where I got this technique.  I've saw
> some web pages mention that Solaris and Windows do it this way.
>
> However, it's not specified, so it's probably a bad idea.  The only way
> I can think to do it another way is to remove the bind() call, then it
> should randomly assign the port (per the spec, I think).  The trouble
> with that is the address will be INADDR_ANY, so it will be bound on
> other interfaces besides the loopback, which make me slightly worried
> from a security point of view.

The other option is to keep the bind but seed it ourselves with a random
port. However then you need to loop until you succeed and avoid locking
up if something fatal has gone wrong.

> I'm ok with this being linux-only, but I'd like to fix it so it works
> everywhere.

One option could be to use the glib networking stuff to set this up.
That has options for creating dynamic ports for incoming connections
although the docs are a bit light on details.
g_socket_listener_add_any_inet_port certainly seems to do the retry
approach (for a weirdly arbitrary 37 times). It leaves the fancy stuff
to a hopefully well tested library although I note QEMU's own use of
g_io APIs seems to be pretty sporadic.


>
> -corey
>
>> working around would add more hard to test complexity to the test I've
>> gone for the easier option of making it CONFIG_LINUX only.
>>
>> Signed-off-by: Alex Bennée <address@hidden>
>> Cc: Corey Minyard <address@hidden>
>> Cc: Kamil Rytarowski <address@hidden>
>> ---
>>  tests/Makefile.include | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/tests/Makefile.include b/tests/Makefile.include
>> index 534ee487436..8566f5f119d 100644
>> --- a/tests/Makefile.include
>> +++ b/tests/Makefile.include
>> @@ -177,7 +177,9 @@ check-qtest-i386-$(CONFIG_SGA) += 
>> tests/boot-serial-test$(EXESUF)
>>  check-qtest-i386-$(CONFIG_SLIRP) += tests/pxe-test$(EXESUF)
>>  check-qtest-i386-y += tests/rtc-test$(EXESUF)
>>  check-qtest-i386-$(CONFIG_ISA_IPMI_KCS) += tests/ipmi-kcs-test$(EXESUF)
>> +ifdef CONFIG_LINUX
>>  check-qtest-i386-$(CONFIG_ISA_IPMI_BT) += tests/ipmi-bt-test$(EXESUF)
>> +endif
>>  check-qtest-i386-y += tests/i440fx-test$(EXESUF)
>>  check-qtest-i386-y += tests/fw_cfg-test$(EXESUF)
>>  check-qtest-i386-y += tests/device-plug-test$(EXESUF)
>> --
>> 2.20.1
>>


--
Alex Bennée