Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test_runtime.sh: Change --rootfs from '' to '.' #113

Merged
merged 1 commit into from
Jun 21, 2016

Conversation

wking
Copy link
Contributor

@wking wking commented Jun 18, 2016

From the spec:

path (string, required) Specifies the path to the root filesystem
for the container. A directory MUST exist at the path declared by
the field.

From POSIX:

The special filename dot shall refer to the directory specified by
its predecessor.

Also from POSIX:

Dot

In the context of naming files, the filename consisting of a single
<period> character ( . ).

Also from POSIX:

Filename

A sequence of bytes consisting of 1 to {NAME_MAX} bytes used to name
a file… The filenames dot and dot-dot have special meaning.

Support for empty-string paths in the wild seems mixed. Coreutils'
realpath complains:

$ realpath ''
realpath: ‘’: No such file or directory
$ realpath --version
realpath (GNU coreutils) 8.23
...
$ lddtree $(command -v realpath)
/usr/bin/realpath (interpreter => /lib64/ld-linux-x86-64.so.2)
    libc.so.6 => /lib64/libc.so.6
$ equery belongs /lib64/libc.so.6
 * Searching for /lib64/libc.so.6 ...
sys-libs/glibc-2.21-r1 (/lib64/libc-2.21.so)
sys-libs/glibc-2.21-r1 (/lib64/libc.so.6 -> libc-2.21.so)

But Bash's cp seems to treat it as .:

$ cd /tmp/
$ pwd
/tmp
$ cd ''
$ pwd
/tmp
$ command -V cd
cd is a shell builtin
$ echo $BASH_VERSION
4.3.42(1)-release

In the test script, the use of the empty string comes from c69f4f1
(Modify default root path to "rootfs", 2015-11-13). The change was
likely because the default 'rootfs' isn't useful when runtimetest
needs to read the config.json from the root of the filesystem, so:

./config.json
./rootfs/runtimetest
./rootfs/…

doesn't work (because after the pivot runtimetest cannot see
config.json). You could copy the config.json:

./config.json
./rootfs/config.json
./rootfs/runtimetest
./rootfs/…

but the current test test script just dispenses with the extra
'rootfs' directory:

./config.json
./runtimetest
./…

Why c69f4f1 switched to an empty string instead of . is unclear,
but I think we should prefer . for the POSIX-compliance reasons
outlined above.

Signed-off-by: W. Trevor King wking@tremily.us

From the spec [1]:

  path (string, required) Specifies the path to the root filesystem
  for the container. A directory MUST exist at the path declared by
  the field.

From POSIX [2]:

  The special filename dot shall refer to the directory specified by
  its predecessor.

Also from POSIX [3]:

  Dot

  In the context of naming files, the filename consisting of a single
  <period> character ( '.' ).

Also from POSIX [4]:

  Filename

  A sequence of bytes consisting of 1 to {NAME_MAX} bytes used to name
  a file... The filenames dot and dot-dot have special meaning.

Support for empty-string paths in the wild seems mixed.  Coreutils'
realpath complains:

  $ realpath ''
  realpath: ‘’: No such file or directory
  $ realpath --version
  realpath (GNU coreutils) 8.23
  ...
  $ lddtree $(command -v realpath)
  /usr/bin/realpath (interpreter => /lib64/ld-linux-x86-64.so.2)
      libc.so.6 => /lib64/libc.so.6
  $ equery belongs /lib64/libc.so.6
   * Searching for /lib64/libc.so.6 ...
  sys-libs/glibc-2.21-r1 (/lib64/libc-2.21.so)
  sys-libs/glibc-2.21-r1 (/lib64/libc.so.6 -> libc-2.21.so)

But Bash's cp seems to treat it as '.':

  $ cd /tmp/
  $ pwd
  /tmp
  $ cd ''
  $ pwd
  /tmp
  $ command -V cd
  cd is a shell builtin
  $ echo $BASH_VERSION
  4.3.42(1)-release

In the test script, the use of the empty string comes from c69f4f1
(Modify default root path to "rootfs", 2015-11-13).  The change was
likely because the default 'rootfs' isn't useful when runtimetest
needs to read the config.json from the root of the filesystem, so:

  ./config.json
  ./rootfs/runtimetest
  ./rootfs/...

doesn't work (because after the pivot runtimetest cannot see
config.json).  You could copy the config.json:

  ./config.json
  ./rootfs/config.json
  ./rootfs/runtimetest
  ./rootfs/...

but the current test test script just dispenses with the extra
'rootfs' directory:

  ./config.json
  ./runtimetest
  ./...

Why c69f4f1 switched to an empty string instead of '.' is unclear,
but I think we should prefer '.' for the POSIX-compliance reasons
outlined above.

[1]: https://github.com/opencontainers/runtime-spec/blob/v1.0.0-rc1/config.md#root-configuration
[2]: http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap04.html#tag_04_12
[3]: http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_136
[4]: http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_170

Signed-off-by: W. Trevor King <wking@tremily.us>
@mrunalp
Copy link
Contributor

mrunalp commented Jun 21, 2016

LGTM

@mrunalp mrunalp merged commit bd9c696 into opencontainers:master Jun 21, 2016
@wking wking deleted the test-runtime-rootfs branch June 21, 2016 22:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants