idl/gen_client: Don't specify argument values when they're equal to defaults

[last-genius]

#6652 added a new parameter to Host.disable. Since the method is used during an RPU, when a new client calls an older server unaware of the parameter, this broke it. Add a test reproducing what happens during an RPU and fix the issue in client.ml.

---

Adds an older server.ml and client.ml from xapi 25.30.0 (with server.ml
modified to compile after the XSA-474 interface changes), before Host.disable
gained the auto_enable parameter.

Adds compatibility tests verifying that an older client can talk to a newer
server and the other way around.

Before the fix, both test_compatibility_with_old_server_* fail, showing that
auto_enable in Host.disable is an unexpected parameter. This failure is
triggered on RPUs, when a newer xapi talks to an older one:

[exception] Server_error(MESSAGE_PARAMETER_COUNT_MISMATCH, [ host.disable; 1; 2 ])

So allow client.ml to skip specifying an arbitrary number of rightmost
arguments if they're all equal to their default values (since arguments are
positional, once an argument is not skipped, no arguments to its left can be
skipped).

Generated code for host.disable looks like the following:

let session_id = rpc_of_ref_session session_id in
let host = rpc_of_ref_host host in
let auto_enable = rpc_of_bool auto_enable in

let needed_args, _ = List.fold_right2
 (fun param default (acc, skipped)->
   (* Since arguments are positional, we can only skip specifying an
      argument that's equal to its default value if all the arguments to
      its right were also not specified *)
   if skipped then
     (match default with
     | Some default_value when param = default_value -> (acc, true)
     | _ -> (param::acc, false))
   else
     (param :: acc, false)
 ) [ session_id; host; auto_enable ] [ None; None; Some (Rpc.Bool true) ] ([], true)
in

rpc_wrapper rpc "host.disable" needed_args >>= fun x -> return (ignore x)

This fixes an issue with client.ml always specifying values for new parameters
that older server.ml did not know about (which happens during an RPU).

This makes test_compatibility_with_old_server_default pass, so drop the
try with for it. test_compatibility_with_old_server_non_default still fails,
indicating that everything works as intended.

Fixes: cf5be62 ("host.disable: Add auto_enabled parameter for persistency")

[last-genius]

No idea what make check doesn't like here if make test is fine with the dependency specified in dune:

File "ocaml/tests/old_server.ml", line 12, characters 59-94:
12 |     List.iter (fun p -> let s = Rpc.to_string p in if not (Xapi_stdext_encodings.Utf8.is_valid s) then
                                                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Error: Unbound module Xapi_stdext_encodings

UPD: seems like there's a different error now that secure_boot field has been added. compare branch tests (https://github.com/last-genius/xen-api/actions/runs/18125555959/job/51579899179) vs. tests in the PR

[robhoes]

+61,494 😳

[robhoes]

Does this also work for the async functions as well, e.g. Async.Host.disable?

[last-genius]

Does this also work for the async functions as well, e.g. Async.Host.disable?

Yes:

      let needed_args, _ = List.fold_right2
        (fun param default (acc, skipped)->
          (* Since arguments are positional, we can only skip specifying an
             argument that's equal to its default value if all the arguments to
             its right were also not specified *)
          if skipped then
            (match default with
            | Some default_value when param = default_value -> (acc, true)
            | _ -> (param::acc, false))
          else
            (param :: acc, false)
        ) [ session_id; host; auto_enable ] [ None; None; Some (Rpc.Bool true) ] ([], true)
      in
      
        rpc_wrapper rpc (Printf.sprintf "%sAsync.host.disable" AQ.async_qualifier) needed_args >>= fun x -> return (ref_task_of_rpc  x)

[robhoes]

I think the new logic here is good. It's a little annoying that for the tests we need to include such large "old" client+server files, but I'm not sure if there is another way to satisfy all the functors and nested modules.

[lindig]

Would this be easier? Generate the full parameter list, reverse it, scan from the front and drop as long as the value is equal to the default value, finally reverse again?

[last-genius]

Test failure shows just how fragile this is... old_server would need to be updated every time a new field is added (or more of its deps imported)

[lindig]

Don't think we want to update the tests as we evolve the API all the time. Would rather see the change in parameter handling as clearly as possible and get it right, never change it again.

[robhoes]

Then I think we take these test as a one-off proof that the new client works, and now leave it out of the PR.

[last-genius]

I've dropped the tests, now have to wait for xs-opam to be fixed.

[lindig]

What is required from xs-opam?

[psafont]

The opam cache was wonky, I deleted it