Re: [PATCH ndctl 2/2] daxctl/device.c: Fix error propagation in do_xaction_device()

[Dan Williams <dan.j.williams@intel.com>]

Vishal Verma wrote:
> The loop through the provided list of devices in do_xaction_device()
> returns the status based on whatever the last device did. Since the
> order of processing devices, especially in cases like the 'all' keyword,
> can be effectively random, this can lead to the same command, and same
> effects, exiting with a different error code based on device ordering.
> 
> This was noticed with flakiness in the daxctl-create.sh unit test. Its
> 'destroy-device all' command would either pass or fail based on the
> order it tried to destroy devices in. (Recall that until now, destroying
> a daxX.0 device would result in a failure).
> 
> Make this slightly more consistent by saving a failed status in
> do_xaction_device if any iteration of the loop produces a failure.
> Return this saved status instead of returning the status of the last
> device processed.

I think "this is the way", at least it follows what cxl/memdev.c is
doing. However we have ended up with an error scheme per tool when it
comes to reporting errors for multi-device operations.

cxl/memdev.c: report the first error

daxctl/device.c: now fixed to report the first error

ndctl/namespace.c: reports last result (same daxctl/device.c issue), unless in the greedy-create case

cxl/region.c: reports the last error even if that is not the last
result, immune to the above bug, but why different?

The struggle here is that all of these tools continue on error, so it
has always been the case that the only way to get a reliable error code
vs action carried out is to not use the "all" or "multi-device" ways to
specify the devices to operate upon.

I don't have a good answer besides, be careful when using "all".

It might make sense to bring ndctl/namespace.c in line to guarantee
"unless 100% of the attempts are successful the command reports
failure". However, it might be too late to make that change there if it
breaks people's scripts. ndctl/namespace.c does not suffer from needing
to know that namspaceX.0 can not be deleted since the deletion there is
exclusively done by setting namespace size to zero.

I think this daxctl change has a low risk of breaking folks because the
primary failure case is fixed to swallow the error.

Reviewed-by: Dan Williams <dan.j.williams@intel.com>