Re: [PATCH v2 07/20] mcd: Implement target initialization API

[Markus Armbruster <armbru@redhat.com>]

Mario Fleischmann <mario.fleischmann@lauterbach.com> writes:

> Thank you very much for the review!
>
> On 08.05.2025 14:04, Markus Armbruster wrote:
>
>> Mario Fleischmann <mario.fleischmann@lauterbach.com> writes:
>> 
>>> The target initialization API ensures that the requested and provided
>>> MCD versions are compatible.
>>>
>>> * implement mcd_initialize_f and mcd_qry_error_info_f in mcdserver
>>> * implement QMP stub functionality
>>> * add QTest
>>>
>>> Thanks to the QMP integration in QTest, function arguments and results
>>> can be (de)serialized automatically.
>>>
>>> Signed-off-by: Mario Fleischmann <mario.fleischmann@lauterbach.com>

[...]

>>> diff --git a/mcd/meson.build b/mcd/meson.build
>>> index 191f0cc..2adaa1b 100644
>>> --- a/mcd/meson.build
>>> +++ b/mcd/meson.build
>>> @@ -1,14 +1,10 @@
>>>  mcd_qapi_outputs = [
>>> -  'mcd-qapi-commands.c',
>>> -  'mcd-qapi-commands.h',
>>>    'mcd-qapi-emit-events.c',
>>>    'mcd-qapi-emit-events.h',
>>>    'mcd-qapi-events.c',
>>>    'mcd-qapi-events.h',
>>>    'mcd-qapi-features.c',
>>>    'mcd-qapi-features.h',
>>> -  'mcd-qapi-init-commands.c',
>>> -  'mcd-qapi-init-commands.h',
>>>    'mcd-qapi-introspect.c',
>>>    'mcd-qapi-introspect.h',
>>>    'mcd-qapi-types.c',
>>> @@ -17,27 +13,48 @@ mcd_qapi_outputs = [
>>>    'mcd-qapi-visit.h',
>>>  ]
>>>  
>>> +# QAPI outputs that will only be used by the MCD server
>>> +mcd_qapi_server_outputs = [
>>> +  'mcd-qapi-commands.c',
>>> +  'mcd-qapi-commands.h',
>>> +  'mcd-qapi-init-commands.c',
>>> +  'mcd-qapi-init-commands.h'
>>> +]
>>> +
>>>  mcd_qapi_files = custom_target('MCD QAPI files',
>>> -                               output: mcd_qapi_outputs,
>>> +                               output: mcd_qapi_outputs + mcd_qapi_server_outputs,
>>>                                 input: '../qapi/mcd.json',
>>>                                 command: [ qapi_gen, '-p', 'mcd-', '-o', 'mcd',
>>>                                            '--suppress-tracing','@INPUT0@'],
>>>                                 depend_files: qapi_gen_depends)
>>>  
>>> -mcd_ss = ss.source_set()
>>> +mcd_qapi_ss = ss.source_set()
>>>  
>>> -mcd_ss.add(mcd_qapi_files.to_list())
>>> -mcd_ss.add(files(
>>> -  'mcd_server.c',
>>> -  'mcd_stub.c',
>>> -  'mcd_monitor.c'))
>>> +foreach f : mcd_qapi_files.to_list()
>>> +  if mcd_qapi_outputs.contains(fs.name(f))
>>> +    mcd_qapi_ss.add([f])
>>> +  endif
>>> +endforeach
>>>  
>>> -mcd_ss = mcd_ss.apply({})
>>> +mcd_qapi_ss.add(files('mcd_qapi.c'))
>>> +mcd_qapi_ss = mcd_qapi_ss.apply({})
>>>  
>>> -libmcd = static_library('mcd_system',
>>> -                        mcd_ss.sources() + genh,
>>> +libmcd_qapi = static_library('mcd_qapi',
>>> +                        mcd_qapi_ss.sources() + genh,
>>>                          build_by_default: false)
>>>  
>>> -mcd = declare_dependency(objects: libmcd.extract_all_objects(recursive: false))
>>> +mcd_qapi = declare_dependency(
>>> +    objects: libmcd_qapi.extract_all_objects(recursive: false))
>>> +
>>> +foreach f : mcd_qapi_files.to_list()
>>> +  if mcd_qapi_server_outputs.contains(fs.name(f))
>>> +    libsystem_ss.add([f])
>>> +  endif
>>> +endforeach
>>> +
>>> +libsystem_ss.add(files(
>>> +  'mcd_server.c',
>>> +  'mcd_stub.c',
>>> +  'mcd_monitor.c'))
>>>  
>>> -system_ss.add(mcd)
>>> +libsystem_ss.add(mcd_qapi)
>> 
>> The commit message did not prepare me for such changes to meson.build.
>> What are you doing here?
>
> I understand that this requires further comments...
> The goal is to provide the marshal helpers from mcd_qapi to mcd-test. We
> cannot use meson's mcd dependency for that since that would result in
> linker errors for functions that are not needed anyway:
>
>   cc -m64  -o tests/qtest/mcd-test [...]
>   build/../mcd/mcd_monitor.c:53: undefined reference to `monitor_puts'
>
> So we split meson's mcd dependency into two parts, one of them being
> mcd_qapi which will be used by mcd-test:
>
>> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
>> index 3dc9508..11a9270 100644
>> --- a/tests/qtest/meson.build
>> +++ b/tests/qtest/meson.build
>> @@ -398,7 +398,7 @@ if dbus_display
>>  endif
>>  
>>  if get_option('mcd').enabled()
>> -  qtests += { 'mcd-test': files('mcd-test.c') }
>> +  qtests += { 'mcd-test': files('mcd-test.c', 'mcd-util.c') + mcd_qapi }
>>    qtests_generic += [ 'mcd-test' ]
>>  endif
>
> I should've probably included this already in
>   [PATCH v2 06/20] qtest: Introduce MCD test suite
> instead of keeping that version's meson files as simple as possible.

Might be less surprising there.

> Provided the explanation above makes sense, I will add a variant of it
> in the commit message for more context.

Yes, please!

> Thanks for pointing that out. After a working a longer stretch on a
> larger patch set, it's quite difficult to see which parts of the
> submission might be unclear.

True!

>>> diff --git a/qapi/mcd.json b/qapi/mcd.json
>>> index 701fd03..7b42a74 100644
>>> --- a/qapi/mcd.json
>>> +++ b/qapi/mcd.json
>>> @@ -4,3 +4,186 @@
>>>  ##
>>>  # = Multi-Core Debug (MCD) API
>>>  ##
>>> +
>>> +
>>> +##
>>> +# == Definition of Structures
>>> +##
>>> +
>>> +
>>> +##
>>> +# @MCDAPIVersion:
>>> +#
>>> +# Structure type containing the MCD API version information of the tool.
>>> +#
>>> +# @v-api-major: API major version.
>>> +# @v-api-minor: API minor version.
>>> +# @author:      API name of the author of this MCD API version.
>>> +#
>>> +# Since: 9.1
>>> +##
>>> +{ 'struct': 'MCDAPIVersion',
>>> +  'data': {
>>> +    'v-api-major': 'uint16',
>>> +    'v-api-minor': 'uint16',
>>> +    'author':      'str' } }
>>> +
>>> +
>>> +##
>>> +# @MCDImplVersionInfo:
>>> +#
>>> +# Structure type containing the MCD API implementation information.
>>> +#
>>> +# @v-api:       Implemented API version.
>>> +# @v-imp-major: Major version number of this implementation.
>>> +# @v-imp-minor: Minor version number of this implementation.
>>> +# @v-imp-build: Build number of this implementation.
>>> +# @vendor:      Name of vendor of the implementation.
>>> +# @date:        String from __DATE__ macro at compile time.
>>> +#
>>> +# Since: 9.1
>>> +##
>>> +{ 'struct': 'MCDImplVersionInfo',
>>> +  'data': {
>>> +    'v-api'      : 'MCDAPIVersion',
>>> +    'v-imp-major': 'uint16',
>>> +    'v-imp-minor': 'uint16',
>>> +    'v-imp-build': 'uint16',
>>> +    'vendor'     : 'str',
>>> +    'date'       : 'str' } }
>>> +
>>> +
>>> +##
>>> +# @MCDErrorInfo:
>>> +#
>>> +# Structure type containing the error status and error event notification.
>>> +#
>>> +# @return-status: Return status from the last API call.
>>> +# @error-code:    Detailed error code from the last API call.
>>> +# @error-events:  Detailed event code from the last API call.
>>> +# @error-str:     Detailed error text string from the last API call.
>>> +#
>>> +# Since: 9.1
>>> +##
>>> +{ 'struct': 'MCDErrorInfo',
>>> +  'data': {
>>> +    'return-status': 'uint32',
>>> +    'error-code'   : 'uint32',
>>> +    'error-events' : 'uint32',
>>> +    'error-str'    : 'str' }}
>>> +
>>> +
>>> +##
>>> +# == Target Initialization API
>>> +##
>>> +
>>> +
>>> +##
>>> +# @MCDInitializeResult:
>>> +#
>>> +# Return value of @mcd-initialize.
>>> +#
>>> +# @return-status: Return code.
>>> +#
>>> +# @impl-info: Information about the QEMU build, its version and the version of
>>> +#             the implemented MCD API.
>>> +#
>>> +# Since: 9.1
>>> +##
>>> +{ 'struct': 'MCDInitializeResult',
>>> +  'data': {
>>> +    'return-status': 'uint32',
>>> +    '*impl-info'   : 'MCDImplVersionInfo' } }
>>> +
>>> +
>>> +##
>>> +# @mcd-initialize:
>>> +#
>>> +# Function initializing the interaction between a tool-side implementation and
>>> +# target-side implementation.
>>> +#
>>> +# @version-req: MCD API version as requested by an upper layer.
>>> +#
>>> +# Returns: @MCDInitializeResult
>>> +#
>>> +# Since: 9.1
>>> +#
>>> +# .. qmp-example::
>>> +#    :title: Check compatibility with MCD server
>>> +#
>>> +#     -> { "execute": "mcd-initialize",
>>> +#          "arguments": { "version-req": { "v-api-major": 1,
>>> +#                                          "v-api-minor": 1,
>>> +#                                          "author": "" } } }
>>> +#     <- {
>>> +#            "return": {
>>> +#                "impl-info": {
>>> +#                    "v-api": {
>>> +#                        "v-api-minor": 1,
>>> +#                        "v-api-major": 1,
>>> +#                        "author": "QEMU Release"
>>> +#                    },
>>> +#                    "vendor": "QEMU",
>>> +#                    "v-imp-minor": 2,
>>> +#                    "v-imp-major": 9,
>>> +#                    "v-imp-build": 0,
>>> +#                    "date": "Dec 18 2024"
>>> +#                },
>>> +#                "return-status": 0
>>> +#            }
>>> +#        }
>>> +##
>>> +{ 'command': 'mcd-initialize',
>>> +  'data': { 'version-req': 'MCDAPIVersion' },
>>> +  'returns': 'MCDInitializeResult' }
>>> +
>>> +
>>> +##
>>> +# @mcd-exit:
>>> +#
>>> +# Function cleaning up all core and server connections from a tool.
>>> +#
>>> +# Since: 9.1
>>> +##
>>> +{ 'command': 'mcd-exit' }
>>> +
>>> +
>>> +##
>>> +# == Core Connection API
>>> +##
>>> +
>>> +
>>> +##
>>> +# @mcd-qry-error-info:
>>> +#
>>> +# Function allowing the access to detailed error and/or event information after
>>> +# an API call.
>>> +#
>>> +# Returns: @MCDErrorInfo
>>> +#
>>> +# Since: 9.1
>>> +#
>>> +# .. qmp-example::
>>> +#    :title: Incompatible MCD versions
>>> +#
>>> +#     -> { "execute": "mcd-initialize",
>>> +#          "arguments": { "version-req": { "v-api-major": 2,
>>> +#                                          "v-api-minor": 0,
>>> +#                                          "author": "" } } }
>>> +#     <- {
>>> +#            "return": {
>>> +#                "return-status": 3
>>> +#            }
>>> +#        }
>>> +#     -> { "execute": "mcd-qry-error-info" }
>>> +#     <- {
>>> +#            "return": {
>>> +#                "error-str": "incompatible versions",
>>> +#                "error-code": 3840,
>>> +#                "error-events": 0,
>>> +#                "return-status": 3
>>> +#            }
>>> +#        }
>>> +##
>>> +{ 'command': 'mcd-qry-error-info',
>>> +  'returns': 'MCDErrorInfo' }
>> 
>> You need "Since: 10.0" now.
>> 
>> From docs/devel/qapi-code-gen.rst:
>> 
>>     For legibility, wrap text paragraphs so every line is at most 70
>>     characters long.
>> 
>> and
>> 
>>     Descriptions start with '\@name:'.  The description text must be
>>     indented like this::
>> 
>>      # @name: Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed
>>      #     do eiusmod tempor incididunt ut labore et dolore magna aliqua.
>> 
>> [...]
>
> Will be changed in v3!

Correction: "Since: 10.1".