Skip to contentSkip to conversation

fix canonicalize-existing vbd file

PATCH In review Akio Takebe 3 participants 7 messages Dec 3, 2007 – Dec 5, 2007

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 fix canonicalize-existing vbd file

Hi,

This is a patch to fix block shell of tools.
The block shell show a wrong message,
when we specify a wrong path of vbd file.

Before applying my patch:

  # ls /root/ttylinux-xen 
  ls: cannot access /root/ttylinux-xen: No such file or directory
  # xm create -c /etc/xen/ttylinux
  Using config file "/etc/xen/ttylinux".
  Error: Device 769 (vbd) could not be connected.
  File /root/ttylinux-xen is read-only, and so I will not
  mount it read-write in a guest domain.

After applying my patch:

  # xm create ttylinux
  Using config file "/etc/xen/ttylinux".
  Error: Device 769 (vbd) could not be connected. /root/ttylinux-xen does not exist.

Best Regards,

Akio Takebe

tools/examples/block +1 -1
diff -r 3057f813da14 tools/examples/block
@@ -219,7 +219,7 @@ case "$command" in
       file)
         # Canonicalise the file, for sharing check comparison, and the mode
         # for ease of use here.
-        file=$(readlink -f "$p") || fatal "$p does not exist."
+        file=$(readlink -e "$p") || fatal "$p does not exist."
         mode=$(canonicalise_mode "$mode")
 
         claim_lock "block"

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

#1 lore raw

The -e option is not available in all versions of readlink.

-- Keir

Quoted 30 lines · Akio Takebe

Hi,

This is a patch to fix block shell of tools.
The block shell show a wrong message,
when we specify a wrong path of vbd file.

Before applying my patch:

  # ls /root/ttylinux-xen
  ls: cannot access /root/ttylinux-xen: No such file or directory
  # xm create -c /etc/xen/ttylinux
  Using config file "/etc/xen/ttylinux".
  Error: Device 769 (vbd) could not be connected.
  File /root/ttylinux-xen is read-only, and so I will not
  mount it read-write in a guest domain.

After applying my patch:

  # xm create ttylinux
  Using config file "/etc/xen/ttylinux".
  Error: Device 769 (vbd) could not be connected. /root/ttylinux-xen does not
exist.

Best Regards,

Akio Takebe
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

#2 lore raw

Hi, Keir

Quoted 1 line

The -e option is not available in all versions of readlink.

Thank you for your review.
How about the blew?

---

tools/examples/block +2 -1
diff -r 3057f813da14 tools/examples/block
@@ -219,7 +219,8 @@ case "$command" in
       file)
         # Canonicalise the file, for sharing check comparison, and the mode
         # for ease of use here.
-        file=$(readlink -f "$p") || fatal "$p does not exist."
+        test -f "$p" || fatal "$p does not exist."
+        file=$(readlink -f "$p")
         mode=$(canonicalise_mode "$mode")
 
         claim_lock "block"
tools/examples/block +2 -1
diff -r 3057f813da14 tools/examples/block
@@ -219,7 +219,8 @@ case "$command" in
       file)
         # Canonicalise the file, for sharing check comparison, and the mode
         # for ease of use here.
-        file=$(readlink -f "$p") || fatal "$p does not exist."
+        test -f "$p" || fatal "$p does not exist."
+        file=$(readlink -f "$p")
         mode=$(canonicalise_mode "$mode")
 
         claim_lock "block"

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

#3 lore raw

Do you mean to 'test -f "$file"'? A symlink will always fail 'test -f', and $p can be a symlink.

-- Keir

Quoted 23 lines · Akio Takebe

Hi, Keir

Quoted 1 line

The -e option is not available in all versions of readlink.

Thank you for your review.
How about the blew?

---
diff -r 3057f813da14 tools/examples/block

tools/examples/block Wed Dec 05 22:57:56 2007 +0900 renamed from tools/examples/block Thu Nov 29 19:30:33 2007 +0000 +2 -1
@@ -219,7 +219,8 @@ case "$command" in
       file)
         # Canonicalise the file, for sharing check comparison, and the mode
         # for ease of use here.
-        file=$(readlink -f "$p") || fatal "$p does not exist."
+        test -f "$p" || fatal "$p does not exist."
+        file=$(readlink -f "$p")
         mode=$(canonicalise_mode "$mode")
 
         claim_lock "block"
#4 lore raw

Keir Fraser writes ("Re: [Xen-devel] [Patch] fix canonicalize-existing vbd file"):

Quoted 2 lines

Do you mean to 'test -f "$file"'? A symlink will always fail 'test -f', and $p can be a symlink.

test -f calls stat(2) rather than lstat(2) and only falls back to the latter if the former fails. So if the argument is a link which can be dereferenced, test -f tells you whether the link target is a file.

If you want to know whether $file is a file or a symlink to a file
test -f "$file"
precisely wrong as it is true in both those cases.

See

    http://www.opengroup.org/onlinepubs/009695399/utilities/test.html

which says
With the exception of the -h file and -L file primaries, if a file argument is a symbolic link, test shall evaluate the expression by resolving the symbolic link and using the file referenced by the link.

It's actually true, too:

mariner:~/junk> touch a
mariner:~/junk> ln -s a b
mariner:~/junk> test -f a && echo file
file
mariner:~/junk> test -f b && echo file
file
mariner:~/junk> test -L b && echo link
link
mariner:~/junk> ln -s enoent c
mariner:~/junk> test -f c && echo file
mariner:~/junk> test -L c && echo link
link
mariner:~/junk>

Ian.

#5 lore raw

Hi, Keir and Ian

Thank you, Ian.
How about the attached new patch?

Best Regards,

Akio Takebe

Quoted 37 lines

Keir Fraser writes ("Re: [Xen-devel] [Patch] fix canonicalize-existing vbd file"):

Quoted 2 lines

Do you mean to 'test -f "$file"'? A symlink will always fail 'test -f', and $p can be a symlink.

test -f calls stat(2) rather than lstat(2) and only falls back to the latter if the former fails. So if the argument is a link which can be dereferenced, test -f tells you whether the link target is a file.

If you want to know whether $file is a file or a symlink to a file
test -f "$file"
precisely wrong as it is true in both those cases.

See
http://www.opengroup.org/onlinepubs/009695399/utilities/test.html
which says
With the exception of the -h file and -L file primaries, if a file argument is a symbolic link, test shall evaluate the expression by resolving the symbolic link and using the file referenced by the link.

It's actually true, too:

mariner:~/junk> touch a
mariner:~/junk> ln -s a b
mariner:~/junk> test -f a && echo file
file
mariner:~/junk> test -f b && echo file
file
mariner:~/junk> test -L b && echo link
link
mariner:~/junk> ln -s enoent c
mariner:~/junk> test -f c && echo file
mariner:~/junk> test -L c && echo link
link
mariner:~/junk>

Quoted 1 line

Ian.

tools/examples/block +1 -0
diff -r 3057f813da14 tools/examples/block
@@ -220,6 +220,7 @@ case "$command" in
         # Canonicalise the file, for sharing check comparison, and the mode
         # for ease of use here.
         file=$(readlink -f "$p") || fatal "$p does not exist."
+        test -f "$file" || fatal "$file does not exist."
         mode=$(canonicalise_mode "$mode")
 
         claim_lock "block"

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

#6 lore raw

Akio Takebe writes ("Re: [Xen-devel] [Patch] fix canonicalize-existing vbd file"):

Quoted 1 line

How about the attached new patch?

...

Quoted 3 lines
         file=$(readlink -f "$p") || fatal "$p does not exist."
+        test -f "$file" || fatal "$file does not exist."
         mode=$(canonicalise_mode "$mode")

That looks reasonable to me.

Ian.

PS: it would be slightly better in general to have error distinct and therefore distinguishable error messages. But that doesn't seem very important here.