Make xe warn about ignored command-line parameters

[shindere]

Before this PR

As reported in this forum post,
a command like

$ xe sr-create name-label=my-iso-sr type=iso \
     device-config:location=192.0.2.1:/isos \
     other-config:auto-scan=true
<sr-uuid>

creates the SR, but neither acts on other-config:auto-scan nor reports that it
went ignored.

After this PR

This PR changes no behaviour until the report is explicitly switched on, by
setting report-ignored-params to warn in one of two ways:

• Server-side — report-ignored-params = warn in /etc/xapi.conf.
• Client-side — report-ignored-params=warn on the xe invocation, or once
  in XE_EXTRA_ARGS / ~/.xe.

A client-side value overrides the server-side one.

With reporting enabled, the command from the previous section writes one more
line:

Ignored parameter: other-config:auto-scan

The ignored parameters are reported on stderr, as diagnostic output usually is.
Because a successful command can have ignored parameters too, this breaks an
xe invariant — that a successful command writes nothing to stderr, which some
scripts rely on. That is why reporting is off by default.

The command's own output, on stdout, is unaffected.

*-list commands are also unchanged: they read every extra key=value as a
record filter (xe vm-list power-state=running), so an unrecognised one is
already a hard error, not an ignored parameter.

How it works

The report is built by tracking which parameters an xe command effectively
reads as it runs.

This is achieved by making the representation of the command line abstract. It
used to be a plain (string * string) list, read directly with List.assoc;
it is now Cli_args.t, whose entries are reachable only through the Cli_args
API. Each accessor — get, get_opt, exists, and so on — sets a used flag
on the entry it touched. After the command runs, Cli_args.unused lists the
parameters that were ignored.

Design choices

Complete over early. The report lists exactly what the command left unread,
so it cannot exist until the command has finished. A check before execution
(against the declared parameter lists in cli_frontend.ml) would catch some
mistakes ahead of any side effect, but only some — it cannot tell that
message-create reads the first of vm-uuid, host-uuid, sr-uuid,
pool-uuid present, so vm-uuid=X sr-uuid=Y silently drops sr-uuid. The
after-the-fact report is the price of catching every case.

Errors caught before the command runs — a command line that does not parse, an
unknown command, a missing required parameter — thus produce no report.

A mode rather than a switch. report-ignored-params takes off or warn
for now. A stricter error — exit non-zero when a parameter was ignored even
though the command itself succeeded — could be added later as a third value,
without a new setting or a rename, and without disturbing the off / warn
already written on command lines and in xapi.conf.

Injected parameters count as used. Some commands add filter parameters the
user never typed — VM selection, for instance, adds is-control-domain=false so
control domains are not matched. Those injected parameters are marked as used. A
parameter the user passed for the same key is not, so in a command that forces
it, is-control-domain=true is still reported as ignored.

progress is kept out of the report; force and multiple are not.
Framework keys — server, username, trace and the rest — are marked as
used: the command does not read them, but the framework does, so they are not
ignored. progress is a real command parameter, and strict accuracy would
report it when a command does not honour it. But it only controls display — a
progress the command does not honour costs nothing more than a missing
progress bar — so it stays out of the report. force and multiple change what
the command does, and a force or multiple the command ignores can mean the
user misunderstood it, so both stay in.

How to review

Best reviewed commit by commit.

1–3 — Cli_frontend tidy-ups. An interface for Cli_frontend — which also
documents parse_commandline's type change — plus a simpler parse_commandline
and removal of the dead rio_help. Unrelated to the feature; done first so they
stay out of the later diffs.

4–7 — preparation for commit 8. The parts of the parameter-handling code
that are not a straight substitution: params_except, override_param and
assoc_default_ci become helpers, and the vm_migrate options block is
rewritten. Isolating them here lets commit 8 be purely mechanical, with nothing
subtle folded in.

8 — route every parameter access through Cli_args. The largest diff, and a
pure mechanical substitution — no behaviour change. List.assoc "x" params
becomes Cli_args.get "x" params, and so on, across cli_operations.ml,
cli_frontend.ml and xapi_cli.ml. The type is abstract; op in cli_cmdtable
now carries string Cli_args.t instead of (string * string) list.

9–11 — map/set plumbing, still no behaviour change:

• Cli_args.view: a lens onto the prefix:key=value map/set entries (and the
  old prefix-key=value form).
• Cli_maps: one named view per map/set parameter, and the one place that lists
  them all.
• an optional ~key for choose_params / select_fields, so vm_disk_list_aux
  stops rebuilding the parameter list just to rename vbd-params / vdi-params.

12 — track reads. Adds the used flag and the marking rules from Design
choices. Wholesale consumers call Cli_args.consume. vm_migrate's parameter
aliasing switches to Cli_args.rename_key, which edits entries in place; the old
code round-tripped through to_pairs / from_pairs and lost the tracking.
Cli_args.unused is defined here but not yet called, so nothing observable
changes.

13 — unit tests for the tracking engine (ocaml/tests/test_cli_args.ml, in
suite_alcotest): which accessors mark, marking seen through a view or a derived
structure, prefix stripping, consume / mark_used / rename_key, and that
unused returns exactly the untouched keys.

14 — the report. report_ignored_params calls unused and prints, from a
finally around the command in do_rpcs, so it fires whether the command
succeeded or failed. Gated on report-ignored-params=warn on the invocation
(also settable via XE_EXTRA_ARGS / ~/.xe).

15 — add xapi.conf as a second place to switch it on. Adds the
report-ignored-params key to xapi.conf as a fallback, consulted only when the
parameter is absent from the invocation.

[gthvn1]

It looks like you forgot to remove this comment as well. It is not related to "get_chunks" if I'm not mistaken.

[psafont]

This is excellent work, the cli code has accumulated a lot of technical debt and is it need of care, this provides quite a bit of it.

While most of my comments are small nits to reduce duplication, I'm worried about the misuse of Cli_args.to_pairs without an entry-marking function. This makes a leaky interface that makes mistakes easy to do, and so I think to_pairs should be removed from the interface.

[psafont]

It might be worth ensuring that the dropped character is : or -.

[psafont]

Suggested change

	let pVS_uuid = try Cli_args.get "pvs-uuid" params with Not_found -> "" in
	let pVS_uuid = try Cli_args.get_default "pvs-uuid" params "" in

[psafont]

Suggested change

	let filter_out pred t =
	{t with pairs= List.filter (fun (k, _) -> not (pred k)) t.pairs}
	let filter_out pred t = filter (Fun.negate pred) t

[psafont]

Suggested change

	let get key t =
	match List.find_opt (fun (k, _) -> k = key) (visible t) with
	| Some (_, e) ->
	mark e ; e.value
	| None ->
	raise Not_found
	let get_opt key t = List.assoc_opt key (visible t)
	let get_opt key t =
	match List.find_opt (fun (k, _) -> k = key) (visible t) with
	| Some (_, e) ->
	mark e ; Some e.value
	| None ->
	None
	let get_opt key t =
	match List.find_opt (fun (k, _) -> k = key) (visible t) with
	| Some (_, e) ->
	mark e ; Some e.value
	| None ->
	None
	let get k t = match get_opt k t with Some v -> v | None -> raise Not_found

(I'm not sure whether the formatter will like the one-liner)

[psafont]

Suggested change

	let exists key t =
	match List.find_opt (fun (k, _) -> k = key) (visible t) with
	| Some (_, e) ->
	mark e ; true
	| None ->
	false
	let exists key t = get_opt key t |> Option.is_some

[psafont]

Can this use of to_pairs be replaced by map_contents as well?

[psafont]

This is another map_contents.

[psafont]

This is another map_contents.

[psafont]

This is another map_contents.

[psafont]

The help messages (cmd_help in cli_frontend and usage in newcli) should also be changed to mention the value off

[gthvn1]

I reviewed all the commits. Lot of stuff there and a lot of details. Only one minor comment otherwise it looks good to me. Great cleanup and improvements! I was able to run tests as well but I didn't test on a real host.

[shindere]

Many thanks for the general feedback. I will do my best to address the individual comments promptly and efficiently. One qustion before I deep-dive (likely from tomorrow onward): I do share the concern about `to_pairs` but have not been able to find a way to not have it. The two places where I was not sure how to do without it were (1) logging and (2) passing the parameters to the commands themselves. Do you have ideas how to tackle these tw?

[psafont]

(1) logging and (2) passing the parameters to the commands themselves.
Do you have ideas how to tackle these two?

I would prefer having two specialized functions that at least have a particular name to warn against using it, unlike the general and seemlesly harmless to_pairs. Maybe for (1) a log_args, and for (2) a arguments_for_command

[shindere]

Guillaume (2026/09/07 06:11 -0700):

@gthvn1 commented on this pull request. __________________________________________________________________ In [1]ocaml/xapi-cli-server/cli_operations.ml: > @@ -130,16 +130,9 @@ let waiter printer rpc session_id params task = ) (fun () -> Client.Task.destroy ~rpc ~session_id ~self:task) -(* Return the list of k=v pairs for maps. - Works for key which is not follow by a ':', - also match old syntax 'device-config-key' for backwards compatability *) (* The [(key, value)] contents of a map-valued parameter [name:key=value], as a It looks like you forgot to remove this comment as well. It is not related to "get_chunks" if I'm not mistaken.

You are right, it should have been dropped as the one above. Now done, thanks!

[shindere]

Pau Ruiz Safont (2026/09/07 07:16 -0700):

In ocaml/xapi-cli-server/cli_operations.ml: > @@ -134,18 +134,15 @@ let waiter printer rpc session_id params task = Works for key which is not follow by a ':', also match old syntax 'device-config-key' for backwards compatability *) let read_map_params name params = + (* [name:key=value] pairs (also the legacy [name-key=value] form), with the + [name:] / [name-] prefix stripped. The contents of a map-valued parameter + are plain data, not tracked CLI arguments. *) let len = String.length name + 1 in It might be worth ensuring that the dropped character is : or -.

A very good idea, thanks. As I understand it this is not really a regression, as the code before this PR was not enforcing : or - as separators either, but I think it is very sensible to improve this here. As this is a change in behaviour, it has been implemented in its own commit. Also, the commit in question appears after all the refactorings once the code has its final shape. A prefixed key like other-configuration:foo=bar, which would previously have been parsed as "other-config" (name of the view), skipped the u, and then parameter "ration:foo" with value "bar" is no longer understood as being part of the "other-config" view and will thus not be passed down to the lower layers. So, when reporting of ignored parameters is enabled, such a key is now properly reported as ignored, which would not have been the case without this suggestion.

In ocaml/xapi-cli-server/cli_operations.ml: > let ref = Client.PCI.get_by_uuid ~rpc ~session_id ~uuid in let result = Client.PCI.get_dom0_access_status ~rpc ~session_id ~self:ref in printer (Cli_printer.PMsg (Record_util.pci_dom0_access_to_string result)) module PVS_site = struct let introduce printer rpc session_id params = - let name_label = List.assoc "name-label" params in - let name_description = - try List.assoc "name-description" params with Not_found -> "" - in - let pVS_uuid = try List.assoc "pvs-uuid" params with Not_found -> "" in + let name_label = Cli_args.get "name-label" params in + let name_description = Cli_args.get_default "name-description" params "" in + let pVS_uuid = try Cli_args.get "pvs-uuid" params with Not_found -> "" in ⬇️ Suggested change - let pVS_uuid = try Cli_args.get "pvs-uuid" params with Not_found -> "" in + let pVS_uuid = try Cli_args.get_default "pvs-uuid" params "" in

Fixed, thanks

In ocaml/xapi-cli-server/cli_args.ml: > +let filter_out pred t = + {t with pairs= List.filter (fun (k, _) -> not (pred k)) t.pairs} ⬇️ Suggested change -let filter_out pred t = - {t with pairs= List.filter (fun (k, _) -> not (pred k)) t.pairs} +let filter_out pred t = filter (Fun.negate pred) t

Applied, thanks

In ocaml/xapi-cli-server/cli_args.ml: > +let get key t = + match List.find_opt (fun (k, _) -> k = key) (visible t) with + | Some (_, e) -> + mark e ; e.value + | None -> + raise Not_found -let get_opt key t = List.assoc_opt key (visible t) +let get_opt key t = + match List.find_opt (fun (k, _) -> k = key) (visible t) with + | Some (_, e) -> + mark e ; Some e.value + | None -> + None ⬇️ Suggested change -let get key t = - match List.find_opt (fun (k, _) -> k = key) (visible t) with - | Some (_, e) -> - mark e ; e.value - | None -> - raise Not_found - -let get_opt key t = List.assoc_opt key (visible t) -let get_opt key t = - match List.find_opt (fun (k, _) -> k = key) (visible t) with - | Some (_, e) -> - mark e ; Some e.value - | None -> - None - +let get_opt key t = + match List.find_opt (fun (k, _) -> k = key) (visible t) with + | Some (_, e) -> + mark e ; Some e.value + | None -> + None + +let get k t = match get_opt k t with Some v -> v | None -> raise Not_found (I'm not sure whether the formatter will like the one-liner)

Applied

In ocaml/xapi-cli-server/cli_args.ml: > +let exists key t = + match List.find_opt (fun (k, _) -> k = key) (visible t) with + | Some (_, e) -> + mark e ; true + | None -> + false ⬇️ Suggested change -let exists key t = - match List.find_opt (fun (k, _) -> k = key) (visible t) with - | Some (_, e) -> - mark e ; true - | None -> - false +let exists key t = get_opt key t |> Option.is_some

Also applied

In ocaml/xapi-cli-server/cli_args.ml: > +(* Mark the entry for [key], if any, as read -- without reading its value. *) +let mark_used key t = + match List.find_opt (fun (k, _) -> k = key) (visible t) with + | Some (_, e) -> + mark e + | None -> + () ⬇️ Suggested change -(* Mark the entry for [key], if any, as read -- without reading its value. *) -let mark_used key t = - match List.find_opt (fun (k, _) -> k = key) (visible t) with - | Some (_, e) -> - mark e - | None -> - () +let mark_used key t = (get_opt key t : 'a entry option) |> ignore

Applied

In ocaml/xapi-cli-server/cli_operations.ml: > -(* This goes through the list of parameters, extracting any of the form map-nam e-key=value *) -(* where map-name is the name of a map in the class. These will be used to set the key-value *) -(* pair in the map. Returns a list of params that didn't fit this form *) +(* The [(key, value)] contents of a map-valued parameter, marked as read, for + forwarding wholesale to the API. *) +let map_contents view = view |> Cli_args.consume |> Cli_args.to_pairs I think it's worth moving map_contents up in the file so params_except also uses this function to reduce the amount of location where Cli_args.to_pairs is used, since it needs to be coupled with some form of marking.

Good idea, thanks. Applied.

It might be even be worth moving this function to Cli_args as well as assoc_default_ci to remove to_pairs from the interface, since it's dangerous due to its need to be paired with a form of entry-marking.

Done the move, but kept to_pairs at this stage as its only current use is for logging. NOt to say that getting rid of it is a bad idea, but rather to say it's not used a lot any more and, ifwe wanted to remove it, it would probably also be nice to remove the keys function. Perhaps these removals or renamings or interface changes could be done as a follow-up?

I see that to_pairs needs to be tested, can the property be tested on map_contents and assoc_default_ci instead?

Replaced test_from_to_pairs by a map_contents test an dadded a test for assoc_default_ci.

In ocaml/xapi-cli-server/cli_operations.ml: > @@ -3279,7 +3292,7 @@ let select_vms ?(include_control_vms = false) ?(include_ template_vms = false) in (* Filter all the records *) List.fold_left filter_records_on_fields all_recs - (Cli_args.to_pairs filter_params) + (Cli_args.to_pairs (Cli_args.consume filter_params)) Can this use of to_pairs be replaced by map_contents as well?

Yes

In ocaml/xapi-cli-server/cli_operations.ml: > + |> Cli_args.consume + |> Cli_args.to_pairs This is another map_contents.

Fixed

In ocaml/xapi-cli-server/cli_operations.ml: > + |> Cli_args.consume + |> Cli_args.to_pairs This is another map_contents.

Fixed, too

In [11]ocaml/xapi-cli-server/cli_operations.ml: > + |> Cli_args.consume + |> Cli_args.to_pairs This is another map_contents.

Also fixed

In [12]ocaml/xapi-consts/constants.ml: > @@ -317,6 +317,13 @@ let owner_key = "owner" (* xapi-cli-server doesn't link xapi-globs *) let use_event_next = ref false +(* What xe does with command-line parameters that the command never read. + Set from xapi.conf (see xapi_globs), read by the CLI server; lives here + because xapi-cli-server doesn't link xapi-globs. *) +type cli_report_ignored_parameters = Off | Warn The help messages (cmd_help in cli_frontend and usage in newcli) should also be changed to mention the value off

Indeed! Done.

[shindere]

Maybe for (1) a log_args [...]

Done — Cli_args.log_args added and to_pairs removed from the interface; details in the thread on cli_operations.ml.

[psafont]

Thank you so much!

[shindere]

Pau Ruiz Safont (2026/09/09 02:53 -0700):

@psafont approved this pull request. Thank you so much!

Thank you for your time and insightful comments!

[BengangY]

A command that aborts part-way has not read the parameters below its abort point, so they are still unmarked and get reported as "Ignored".
An example:

# xe vdi-create sr-uuid=00000000 name-label=disk1 virtual-size=8GiB type=user sm-config:foo=bar report-ignored-params=warn
Ignored parameter: type
Ignored parameter: sm-config:foo
The uuid you supplied was invalid.
type: SR
uuid: 00000000

[shindere]

Well spotted, many thanks for having reported this -- I should have
documented it explicitly for the sake of clarity.

My understanding is that the current behaviour is correct, in the sense
that it just tells a user that a parameter that was given has actually
not been consumed, for whatever reason. Do you request a change either
in the code or in the documentation?