qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH] qemu-iotests: convert `pwd` and $(pwd) to $PWD


From: maozy
Subject: Re: [Qemu-devel] [PATCH] qemu-iotests: convert `pwd` and $(pwd) to $PWD
Date: Wed, 24 Oct 2018 17:57:40 +0800
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.0

Hi, Eric

On 10/23/18 4:21 PM, Eric Blake wrote:
On 10/22/18 2:48 PM, Mao Zhongyi wrote:

The subject line says "what", but the commit body should say "why".  My suggestion:

POSIX requires $PWD to be reliable, and we expect all shells used by qemu scripts to be relatively close to POSIX.  Thus, it is smarter to avoid forking the pwd executable for something that is already available in the environment.



If it was done mechanically, it may also help to capture the command you used to drive the change (sed or otherwise), to make it easier for someone backporting this patch to rerun the same steps to regenerate the patch for a different set of files on the backport.

Suggested-by: Eric Blake <address@hidden>
Signed-off-by: Mao Zhongyi <address@hidden>
---

+++ b/tests/qemu-iotests/001
@@ -24,7 +24,7 @@ address@hidden
  seq=`basename $0`
  echo "QA output created by $seq"
-here=`pwd`
+here=$PWD

As this is a mechanical search-and-replace, this is fine.  However,
  git grep '\$here' tests/qemu-iotests
has 0 hits, which means we are setting a variable that has no use.  A good followup patch would be to delete all of the 'here=...' lines as dead code. Or even do that first, and then this patch second, for less churn.

+++ b/tests/qemu-iotests/check
@@ -80,17 +80,17 @@ _full_imgfmt_details()
  _full_platform_details()
  {
-    os=`uname -s`
-    host=`hostname -s`
-    kernel=`uname -r`
-    platform=`uname -m`
+    os=$(uname -s)
+    host=$(hostname -s)
+    kernel=$(uname -r)
+    platform=$(uname -m)
      echo "$os/$platform $host $kernel"

These changes are unrelated to the commit subject. Please split them into a separate commit...

  }
  # $1 = prog to look for
  set_prog_path()
  {
-    p=`command -v $1 2> /dev/null`
+    p=$(command -v $1 2> /dev/null)
      if [ -n "$p" -a -x "$p" ]; then
          type -p "$p"
      else
@@ -99,7 +99,7 @@ set_prog_path()
  }
  if [ -z "$TEST_DIR" ]; then
-        TEST_DIR=`pwd`/scratch
+        TEST_DIR=$PWD/scratch
  fi

...This hunk is okay, but the rest of the file is not.


+++ b/tests/qemu-iotests/common.config
@@ -21,11 +21,11 @@ export LANG=C
  PATH=".:$PATH"
-HOSTOS=`uname -s`
-arch=`uname -m`
+HOSTOS=$(uname -s)
+arch=$(uname -m)

Another file with too many hunks.

Looking forward to v2.

Thank you very much for your kind reply, it's updated
in v2, please review. :)

Thanks,
mao







reply via email to

[Prev in Thread] Current Thread [Next in Thread]