nvme_copy_test fails on decoding output

[igaw]

root@localhost:/home/wagi/work/nvme-cli# /usr/bin/nose2 --verbose --start-dir /home/wagi/work/nvme-cli/tests nvme_copy_test
test_copy (nvme_copy_test.TestNVMeCopy.test_copy)
Testcase main ...
Using nvme binary 'nvme'

setup: ctrl: /dev/nvme0, ns1: /dev/nvme0n1, default_nsid: 1, flbas: 0

ERROR



======================================================================
ERROR: test_copy (nvme_copy_test.TestNVMeCopy.test_copy)
Testcase main
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/wagi/work/nvme-cli/tests/nvme_copy_test.py", line 53, in setUp
    if self.host_behavior_data[4] & cross_namespace_copy:
       ~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~
TypeError: unsupported operand type(s) for &: 'str' and 'int'

----------------------------------------------------------------------
Ran 1 test in 0.336s

[igaw]

@MaisenbacherD I was not able to figure it out quickly. Maybe you know what to do here?

[ikegami-t]

Seems the str can be converted to int as below.

-     if self.host_behavior_data[4] & cross_namespace_copy:
+     if int(self.host_behavior_data[4]) & cross_namespace_copy:

[igaw]

As I tried to say, it's not that easy:

Traceback (most recent call last): File "/home/wagi/work/nvme-cli/tests/nvme_copy_test.py", 
line 52, in setUp val = int(self.host_behavior_data[4]) 
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ValueError: invalid literal for int() with base 10: '\x00'

[MaisenbacherD]

We are trying to parse the 4th colum of the get_features_cmd output which tells us if cross_namespace_copy is already enabled. If I am not mistaken this is a hex value, so slightly different from what @ikegami-t proposed:

-     if self.host_behavior_data[4] & cross_namespace_copy:
+     if int(self.host_behavior_data[4], 16) & cross_namespace_copy:

[ikegami-t]

The value is a binary data so seems can be converted to the int value as below. Can you please retry this?

-     if self.host_behavior_data[4] & cross_namespace_copy:
+     if int.from_bytes(self.host_behavior_data[4]) & cross_namespace_copy:

[ikegami-t]

Still the error caused by the above changes then the erorr itself can be resolved by the changes below and seems it works correctly.

 import subprocess
+import base64
...
-            if self.host_behavior_data[4] & cross_namespace_copy:
+            if int.from_bytes(base64.b64decode(self.host_behavior_data[4])) & cross_namespace_copy:

But now still the other error below caused.

ERROR: test_copy (nvme_copy_test.TestNVMeCopy.test_copy)
Testcase main
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/tokunori/nvme-cli-2/tests/nvme_copy_test.py", line 68, in setUp
    data = self.host_behavior_data[:4] + cross_namespace_copy.to_bytes(2, 'little') + self.host_behavior_data[6:]
           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
TypeError: can only concatenate str (not "bytes") to str

[ikegami-t]

The other error can be avoided by the change below but not sure if this correctly works or not.

-                data = self.host_behavior_data[:4] + cross_namespace_copy.to_bytes(2, 'little') + self.host_behavior_data[6:]
+                data = self.host_behavior_data[:4] + str(cross_namespace_copy.to_bytes(2, 'little')) + self.host_behavior_data[6:]

[igaw]

Why not add the missing command/feature instead of doing this rather nasty

            get_features_cmd = f"{self.nvme_bin} get-feature {self.ctrl} " + \
                "--feature-id=0x16 --data-len=512 --raw-binary"

We should have a proper way to retrieve the 'enable cross-namespace copy formats' info and not parse raw outputs...

[ikegami-t]

We can see the field values output as below with the human-readable, verbose or json option instead of the raw-binary.

	case NVME_FEAT_FID_HOST_BEHAVIOR:
		if (buf) {
			struct nvme_feat_host_behavior *host_behavior =
				(struct nvme_feat_host_behavior *)buf;
			printf("\tAdvanced Command Retry Enable (ACRE)                    : %s\n",
			       host_behavior->acre ? "True" : "False");
			printf("\tExtended Telemetry Data Area 4 Supported (ETDAS)        : %s\n",
			       host_behavior->etdas ? "True" : "False");
			printf("\tLBA Format Extension Enable (LBAFEE)                    : %s\n",
			       host_behavior->lbafee ? "True" : "False");
			printf("\tHost Dispersed Namespace Support (HDISNS)               : %s\n",
			       host_behavior->hdisns ? "Enabled" : "Disabled");
			printf("\tCopy Descriptor Format 2h Enabled (CDF2E)               : %s\n",
			       host_behavior->cdfe & (1 << 2) ? "True" : "False");
			printf("\tCopy Descriptor Format 3h Enabled (CDF3E)               : %s\n",
			       host_behavior->cdfe & (1 << 3) ? "True" : "False");
			printf("\tCopy Descriptor Format 4h Enabled (CDF4E)               : %s\n",
			       host_behavior->cdfe & (1 << 4) ? "True" : "False");
		}
		break;

By the way previously the current nvme_copy_test should be worked but I am wondering if the fails caused by the python behavior changed.

[igaw]

The Python code seems to run fine within the CI containers (Debian) but not when I run it on my Fedora box. Likely due to different Python versions v3.13 vs v3.14. Wouldn't be the first time.

Yes, let's go with parsing the JSON output.

BTW, when looking through the code I found this here:

static void get_feature_id_print(struct feat_cfg cfg, int err, __u64 result,
		void *buf, nvme_print_flags_t flags)
{
	int status = filter_out_flags(err);
	int verbose = flags & VERBOSE;
	enum nvme_status_type type = NVME_STATUS_TYPE_NVME;

	if (!err) {
		if (!cfg.raw_binary || !buf) {
			nvme_feature_show(cfg.feature_id, cfg.sel, result);
			if (NVME_CHECK(cfg.sel, GET_FEATURES_SEL, SUPPORTED))
				nvme_show_select_result(cfg.feature_id, result);
			else if (verbose || !strcmp(nvme_args.output_format, "json"))
				nvme_feature_show_fields(cfg.feature_id, result, buf);
			else if (buf)
				d(buf, cfg.data_len, 16, 1);
		} else if (buf) {
			d_raw(buf, cfg.data_len);
		}
	} else if (err > 0) {
		if (!nvme_status_equals(status, type, NVME_SC_INVALID_FIELD) &&
		    !nvme_status_equals(status, type, NVME_SC_INVALID_NS))
			nvme_show_status(err);
	} else {
		nvme_show_error("get-feature: %s", libnvme_strerror(err));
	}
}

This function looks a bit wierd to me. I thought we are handling the output format in the nvme-print.c layer. --raw-binary would just select the binary output format. And also the error handling could be updated to use our new error handling helpers.

[ikegami-t]

Yes I will try to use JSON output and fix the get features command printf fucntion later. Thank you.