[libvirt RFC v3 06/19] remote: Add RPC support for the virDomainRestoreParametersFlags API

[Claudio Fontana]

On 4/28/22 11:34 AM, Daniel P. Berrangé wrote:
> On Wed, Apr 27, 2022 at 10:56:22PM +0200, Claudio Fontana wrote:
>> On 4/27/22 1:00 AM, Jim Fehlig wrote:
>>> On 4/26/22 10:47, Claudio Fontana wrote:
>>>> Signed-off-by: Claudio Fontana <cfontana at suse.de>
>>>> ---
>>>>   src/remote/remote_driver.c   |  1 +
>>>>   src/remote/remote_protocol.x | 14 +++++++++++++-
>>>>   src/remote_protocol-structs  |  8 ++++++++
>>>>   3 files changed, 22 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
>>>> index 1fc5d41971..c5b644ce49 100644
>>>> --- a/src/remote/remote_driver.c
>>>> +++ b/src/remote/remote_driver.c
>>>> @@ -8449,6 +8449,7 @@ static virHypervisorDriver hypervisor_driver = {
>>>>       .domainSaveParametersFlags = remoteDomainSaveParametersFlags, /* 8.3.0 */
>>>>       .domainRestore = remoteDomainRestore, /* 0.3.0 */
>>>>       .domainRestoreFlags = remoteDomainRestoreFlags, /* 0.9.4 */
>>>> +    .domainRestoreParametersFlags = remoteDomainRestoreParametersFlags, /* 8.3.0 */
>>>>       .domainSaveImageGetXMLDesc = remoteDomainSaveImageGetXMLDesc, /* 0.9.4 */
>>>>       .domainSaveImageDefineXML = remoteDomainSaveImageDefineXML, /* 0.9.4 */
>>>>       .domainCoreDump = remoteDomainCoreDump, /* 0.3.0 */
>>>> diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x
>>>> index c2ae5c5748..7b919ef375 100644
>>>> --- a/src/remote/remote_protocol.x
>>>> +++ b/src/remote/remote_protocol.x
>>>> @@ -3236,6 +3236,11 @@ struct remote_domain_save_parameters_flags_args {
>>>>       unsigned int flags;
>>>>   };
>>>>   
>>>> +struct remote_domain_restore_parameters_flags_args {
>>>> +    remote_typed_param params<REMOTE_DOMAIN_SAVE_PARAMS_MAX>;
>>>> +    unsigned int flags;
>>>> +};
>>>> +
>>>>   /* The device removed event is the last event where we have to support
>>>>    * dual forms for back-compat to older clients; all future events can
>>>>    * use just the modern form with callbackID.  */
>>>> @@ -6935,5 +6940,12 @@ enum remote_procedure {
>>>>        * @generate: both
>>>>        * @acl: domain:hibernate
>>>>        */
>>>> -    REMOTE_PROC_DOMAIN_SAVE_PARAMETERS_FLAGS = 440
>>>> +    REMOTE_PROC_DOMAIN_SAVE_PARAMETERS_FLAGS = 440,
>>>> +
>>>> +    /**
>>>> +     * @generate: both
>>>> +     * @acl: domain:start
>>>> +     * @acl: domain:write
>>>> +     */
>>>> +    REMOTE_PROC_DOMAIN_RESTORE_PARAMETERS_FLAGS = 441
>>>
>>> I've stared at this for quite a while but can't figure out why the dispatch stub 
>>> does not pass virConnectPtr to virDomainRestoreParametersFlags. I'm hitting the 
>>> following build failure
>>>
>>> In file included from ../src/remote/remote_daemon_dispatch.c:133:
>>>
>>> src/remote/remote_daemon_dispatch_stubs.h: In function 
>>> ‘remoteDispatchDomainRestoreParametersFlags’:
>>>
>>> src/remote/remote_daemon_dispatch_stubs.h:10080:41: error: passing argument 1 of 
>>> ‘virDomainRestoreParametersFlags’ from incompatible pointer type 
>>> [-Werror=incompatible-pointer-types]
>>>
>>> 10080 |     if (virDomainRestoreParametersFlags(params, nparams, args->flags) < 0)
>>>
>>>        |                                         ^~~~~~
>>>
>>>        |                                         |
>>>
>>>        |                                         virTypedParameterPtr {aka 
>>> struct _virTypedParameter *}
>>>
>>> In file included from ../include/libvirt/libvirt.h:36,
>>>
>>>                   from ../src/internal.h:65,
>>>
>>>                   from ../src/util/virerror.h:24,
>>>
>>>                   from ../src/remote/remote_daemon_dispatch.c:23:
>>>
>>> ../include/libvirt/libvirt-domain.h:1576:72: note: expected ‘virConnectPtr’ {aka 
>>> ‘struct _virConnect *’} but argument is of type ‘virTypedParameterPtr’ {aka 
>>> ‘struct _virTypedParameter *’}
>>>
>>>   1576 | int                     virDomainRestoreParametersFlags (virConnectPtr 
>>> conn,
>>>
>>>
>>> Perhaps a bug in gendispatch.pl. I'm not familiar with the script or debugging 
>>> it, but others here can likely provide help.
>>>
>>> Jim
>>>
>>
>> Still fighting this one, could not defeat the beast yet..
> 
> Don't bother. gendispatch.pl is not capable of correctly handling all
> API designs we have and it isn't worth trying to fix it unless the
> problem is obvious or you enjoy reading & writing obtuse perl code ;-P
> 
> In the .xdr file, switch 'generate: both' to exclude either the client
> or server code generation, or both, depending on which bit is broken.
> Then just write the code by hand. You can start with the broken code
> and just fix it up and add to remote_daemon_dispatch.c directly. You
> will see several examples not using 'generate: both' you can follow
> 
> 
> With regards,
> Daniel

I actually think I found the problem. It needs to be put explicitly into a list
of methods that require conn, like NodeSetMemoryParameters.

Here is the change:

commit 192d8e56e88c00ede94768f6f73c6be64f31c789 (HEAD -> api_draft)
Author: Claudio Fontana <cfontana at suse.de>
Date:   Thu Apr 28 08:16:27 2022 -0600

    gendispatch: add DomainRestoreParametersFlags as requiring conn argument
    
    Signed-off-by: Claudio Fontana <cfontana at suse.de>

diff --git a/src/rpc/gendispatch.pl b/src/rpc/gendispatch.pl
index 9f5bf0e316..bce88cfc52 100755
--- a/src/rpc/gendispatch.pl
+++ b/src/rpc/gendispatch.pl
@@ -637,7 +637,10 @@ elsif ($mode eq "server") {
                 } elsif ($args_member =~ m/^remote_typed_param (\S+)<(\S+)>;/) {
                     push(@vars_list, "virTypedParameterPtr $1 = NULL");
                     push(@vars_list, "int n$1 = 0");
-                    if ($call->{ProcName} eq "NodeSetMemoryParameters") {
+
+                    # NB: if your new API starts with remote_typed_params, enter it here if you need
+                    # the conn arg to be passed first!
+                    if ($call->{ProcName} eq "NodeSetMemoryParameters" || $call->{ProcName} eq "DomainRestoreParametersFlags") {
                         push(@args_list, $conn_var);
                     }
                     push(@args_list, "$1");