Re: [Qemu-devel] [PULL v3 00/51] Ivshmem patches

[John Snow]

On 10/13/2015 01:16 PM, Marc-André Lureau wrote:
> 
> 
> ----- Original Message -----
>>
>>
>> On 10/13/2015 12:10 PM, Marc-André Lureau wrote:
>>> Hi
>>>
>>> ----- Original Message -----
>>>> On 13 October 2015 at 15:25,  <address@hidden> wrote:
>>>>> From: Marc-André Lureau <address@hidden>
>>>>>
>>>>> The following changes since commit
>>>>> c49d3411faae8ffaab8f7e5db47405a008411c10:
>>>>>
>>>>>   Merge remote-tracking branch 'remotes/armbru/tags/pull-qapi-2015-10-12'
>>>>>   into staging (2015-10-13 10:42:06 +0100)
>>>>>
>>>>> are available in the git repository at:
>>>>>
>>>>>   address@hidden:elmarco/qemu.git tags/ivshmem-pull-request
>>>>>
>>>>> for you to fetch changes up to feb3f96c4ff1613dd4d0bebda09fe349f8c3e2dd:
>>>>>
>>>>>   doc: document ivshmem & hugepages (2015-10-13 15:29:53 +0200)
>>>>>
>>>>> ----------------------------------------------------------------
>>>>> v3 with build fixes on osx & x86
>>>>> ----------------------------------------------------------------
>>>>
>>>> This asserts in the tests on OSX:
>>>>
>>>> GTESTER check-qtest-i386
>>>> blkdebug: Suspended request 'A'
>>>> blkdebug: Resuming request 'A'
>>>> Using POSIX shared memory: /qtest-68262-3644687833
>>>> ftruncate(/qtest-68262-3644687833) failed: Invalid argument
>>>> **
>>>
>>> I'll try to reproduce on freebsd. It's weird that this ftruncate() would
>>> fail on osx but not on linux, perhaps a osx security?
>>>
>>>> ERROR:/Users/pm215/src/qemu-for-merges/tests/ivshmem-test.c:299:void
>>>> test_ivshmem_server(): assertion failed (ret == 0): (-1 == 0)
>>>> GTester: last random seed: R02S141a4c6774f852248b61ebcd666b7ad5
>>>>
>>>> (I'm afraid I didn't notice this in earlier testing because
>>>> for some reason I'm not clear on an assertion failure doesn't
>>>> always cause the test harness to fail.)
>>>>
>>>> Some asides, which you should look into but which don't need
>>>> to be fixed for this pull request:
>>>>  * having the test use 'is QTEST_LOG set' as its "should we be verbose
>>>> in the server failure path" is not terribly helpful because QTEST_LOG
>>>> enables vast volumes of libqtest tracing of communications between
>>>> qemu and the test harness, and anything else is lost in the noise
>>>
>>> What do you suggest instead?
>>>
>>
>> https://developer.gnome.org/glib/stable/glib-Testing.html#g-test-verbose
> 
> Yes, but the question is why we have QTEST_LOG then :) I don't mind much 
> using g_test_verbose(), except it is a different way to enable logging, also 
> it could spew a lot of debug too :)
> 

In my mind, verbose was for printing extra test messages, but QTEST_LOG
was for printing qemu-qtest interactions and messages passed over the
qtest socket.

If you're worried about using the standard gtester verbose flag (and
producing too much output,) you could just add your own verbose flag to
the qtest if you wanted to and it would turn into a developer-only flag
of sorts for just that one test.

You can also use g_test_message() which will be hidden by default and
revealed by --verbose.

Up to you.

>>
>>>>  * ivshmem_server_init() has uses of IVSHMEM_SERVER_DEBUG before the
>>>> verbose flag is copied into server->verbose, which means they won't
>>>> print things out when they should
>>>
>>> True, I moved it up.
>>>
>>>>  * ivshmem_server_start() is inconsistent about whether it wants
>>>> to report "something failed messages to stderr or via the debug macro
>>>
>>> stderr is for fatal errors. Only gethugepagesize() that I added uses stderr
>>> too because it's a generic function. I'll make it take IvshmemServer
>>> argument and use IVSHMEM_SERVER_DEBUG.
>>>
>>>>  * ivshmem_server_ftruncate() is using some bizarre code to
>>>> align up to a power of 2. We have pow2ceil() for this
>>>
>>> ok
>>>
>>>>  * Printing "Using POSIX shared memory" in the test output for a
>>>> normal non-verbose test run isn't great: generally our tests should
>>>> be silent except regarding failures
>>>
>>> This was added by earlier reviewer request. I guess during make check,
>>> there could be a way to make it silent instead.
>>>
>>> thanks
>>>
>>
>>

-- 
—js