Re: [PATCH] selftests: firmware: Add details in error logging

On Fri, 23 May 2025 at 03:49, Shuah Khan skhan@linuxfoundation.org wrote:

On 5/16/25 09:39, Harshal wrote:

...

Specify details in logs of failed cases

Use die() instead of exit() when write to sys_path fails

Please explain why this change is needed?

...

Signed-off-by: Harshal embedkari167@gmail.com

tools/testing/selftests/firmware/fw_namespace.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/tools/testing/selftests/firmware/fw_namespace.c b/tools/testing/selftests/firmware/fw_namespace.c index 04757dc7e546..deff7f57b694 100644 --- a/tools/testing/selftests/firmware/fw_namespace.c +++ b/tools/testing/selftests/firmware/fw_namespace.c @@ -38,10 +38,11 @@ static void trigger_fw(const char *fw_name, const char *sys_path)

  fd = open(sys_path, O_WRONLY);
  if (fd < 0)

•         die("open failed: %s\n",
  

•         die("open of sys_path failed: %s\n",
              strerror(errno));
  if (write(fd, fw_name, strlen(fw_name)) != strlen(fw_name))
  

•         exit(EXIT_FAILURE);
  

•         die("write to sys_path failed: %s\n",
  

•             strerror(errno));
  

Hmm. Wrapper scripts key off of the EXIT_FAILURE - how does the output change with this change?

In the whole test file, all fail cases uses die() with the specified error log and error code, so just wanted to add a error log for this fault condition and maintain consistency.

...

  close(fd);

}

@@ -52,10 +53,10 @@ static void setup_fw(const char *fw_path)

  fd = open(fw_path, O_WRONLY | O_CREAT, 0600);
  if (fd < 0)

•         die("open failed: %s\n",
  

•         die("open of firmware file failed: %s\n",
              strerror(errno));
  if (write(fd, fw, sizeof(fw) -1) != sizeof(fw) -1)
  

•         die("write failed: %s\n",
  

•         die("write to firmware file failed: %s\n",
              strerror(errno));
  close(fd);
  

  }

@@ -66,7 +67,7 @@ static bool test_fw_in_ns(const char *fw_name, const char *sys_path, bool block_

  if (block_fw_in_parent_ns)
          if (mount("test", "/lib/firmware", "tmpfs", MS_RDONLY, NULL) == -1)

•                 die("blocking firmware in parent ns failed\n");
  

•                 die("blocking firmware in parent namespace failed\n");
  
  child = fork();
  if (child == -1) {
  

@@ -99,11 +100,11 @@ static bool test_fw_in_ns(const char *fw_name, const char *sys_path, bool block_ strerror(errno)); } if (mount(NULL, "/", NULL, MS_SLAVE|MS_REC, NULL) == -1)

•         die("remount root in child ns failed\n");
  

•         die("remount root in child namespace failed\n");
  
  if (!block_fw_in_parent_ns) {
          if (mount("test", "/lib/firmware", "tmpfs", MS_RDONLY, NULL) == -1)
  

•                 die("blocking firmware in child ns failed\n");
  

•                 die("blocking firmware in child namespace failed\n");
  } else
          umount("/lib/firmware");
  
  

@@ -129,8 +130,8 @@ int main(int argc, char **argv) die("error: failed to build full fw_path\n");

  setup_fw(fw_path);

• setvbuf(stdout, NULL, _IONBF, 0);
  

• /* Positive case: firmware in PID1 mount namespace */
  printf("Testing with firmware in parent namespace (assumed to be same file system as PID1)\n");
  if (!test_fw_in_ns(fw_name, sys_path, false))
  

The rest look fine.

thanks, -- Shuah