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

reimplement logging without referencing file descriptors #78

Merged
merged 3 commits into from
Dec 6, 2016

Conversation

fwyzard
Copy link
Contributor

@fwyzard fwyzard commented Oct 15, 2016

reimplenent the verbose and debug messages without referencing /dev/stdout or
/dev/stderr, which may be unavailable (e.g. running inside a container)

@cmsbuild
Copy link

A new Pull Request was created by @fwyzard (Andrea Bocci) for branch master.

@cmsbuild, @smuzaffar, @iahmad-khan, @davidlange6 can you please review it and eventually sign? Thanks.
You can sign-off by replying to this message having '+1' in the first line of your reply.
You can reject by replying to this message having '-1' in the first line of your reply.

external issue cms-sw/cmsdist#2587

reimplenent the verbose and debug messages without referencing /dev/stdout or
/dev/stderr, which may be unavailable (e.g. running inside a container)
@fwyzard
Copy link
Contributor Author

fwyzard commented Dec 1, 2016

please test

@fwyzard
Copy link
Contributor Author

fwyzard commented Dec 1, 2016

@smuzaffar , is anything holding this up ?

reimplenent the verbose and debug messages without referencing /dev/stdout or
/dev/stderr, which may be unavailable (e.g. running inside a container)
@smuzaffar
Copy link
Contributor

nothing , just that did not find much time to go through the changes. Only way to test these is to put in DEVEL IBs and see how it goes

@cmsbuild
Copy link

cmsbuild commented Dec 1, 2016

Pull request #78 was updated.

external issue cms-sw/cmsdist#2587

@fwyzard
Copy link
Contributor Author

fwyzard commented Dec 1, 2016

OK, I've migrated also git-cms-merge-topic and git-cms-sparse-checkout and I've tested in a container where /dev/stderr does not work.

@davidlt
Copy link

davidlt commented Dec 1, 2016

Not having /dev/stdout and /dev/stderr is very unlikely. Which container backend doesn't bind-mount these?

@fwyzard
Copy link
Contributor Author

fwyzard commented Dec 1, 2016

They exist, but if you enter the container from a local shell (instead of e.g. over ssh) they point to devices that the local user does not have access to:

fwyzard@fool:~$ lxc exec slc6 -- /usr/bin/sudo -i -u fwyzard
fwyzard@slc6:~$ ls -l /dev/stderr 
lrwxrwxrwx 1 root root 15 Dec  1 13:26 /dev/stderr -> /proc/self/fd/2
fwyzard@slc6:~$ echo 'Hello world'
Hello world
fwyzard@slc6:~$ echo 'Hello world' > /dev/stderr 
-bash: /dev/stderr: Permission denied


if [ $VERBOSE == 0 ]; then
# send vrbose messages to /dev/null
exec {verbose}> /dev/null
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you point to bash documentation about this?
I found that on Bash 3.X (OS X) it doesn't work: ./test2.sh: line 6: exec: {verbose}: not found
I couldn't find anything on Bash manual on this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

correct, looks like exec {verbose}> /dev/null type of commands are not working on OS X

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

too nice to be true... ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from man bash and https://www.gnu.org/software/bash/manual/bash.html#Redirections

Each redirection that may be preceded by a file descriptor number may instead be preceded by a word of the form {varname}. In this case, for each redirection operator except >&- and <&-, the shell will allocate a file descriptor greater than or equal to 10 and assign it to varname. If >&- or <&- is preceded by {varname}, the value of varname defines the file descriptor to close.

The use of exec for redirections is documented in many online articles, e.g. http://www.linuxjournal.com/content/bash-redirections-using-exec .

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I knew that you can do that with exec using file descriptor numbers, but man pages didn't specify that {varname}, but it's is mentioned in info pages. Good to know.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going through the history of the bash documentation, looks like this was introduced (or at least documented) in bash 4.1

@davidlt
Copy link

davidlt commented Dec 1, 2016

I guess, you are on very old LXC. This looks to fixed almost 3 years ago.

@fwyzard
Copy link
Contributor Author

fwyzard commented Dec 1, 2016

I guess, you are on very old LXC. This looks to fixed almost 3 years ago.

I'm using LXD 2.5, released on October 26th: https://github.com/lxc/lxd/releases/tag/lxd-2.5 .

@davidlt
Copy link

davidlt commented Dec 1, 2016

I am running LXC 2.0.5 (Fedora 25). Create Debian container:

[davidlt@pccms205 ~]$ sudo lxc-attach -n davidlt2
root@davidlt2:/# ls /dev/
console  core  fd  full  hugepages  initctl  log  lxc  mqueue  null  ptmx  pts  random  shm  stderr  stdin  stdout  tty  tty1  tty2  tty3  tty4  urandom  zero
root@davidlt2:/# file /dev/stderr
/dev/stderr: symbolic link to /proc/self/fd/2
root@davidlt2:/# file /dev/stdout
/dev/stdout: symbolic link to /proc/self/fd/1

@fwyzard
Copy link
Contributor Author

fwyzard commented Dec 1, 2016

As I posted above, I do see /dev/stdout (and /dev/stderr):

fwyzard@fool:~/src/bash$ lxc exec slc6 -- /usr/bin/sudo -i -u fwyzard
fwyzard@slc6:~$ file /dev/stdout
/dev/stdout: symbolic link to `/proc/self/fd/1'
fwyzard@slc6:~$ file /dev/stderr
/dev/stderr: symbolic link to `/proc/self/fd/2'

but the shell is not able to write to them

fwyzard@slc6:~$ echo 'Hello world' > /dev/stdout
-bash: /dev/stdout: Permission denied
fwyzard@slc6:~$ echo 'Hello world' > /dev/stderr
-bash: /dev/stderr: Permission denied

probably because the underlying /dev/pts is not available inside the container

fwyzard@slc6:~$ file /proc/self/fd/1
/proc/self/fd/1: broken symbolic link to `/dev/pts/2'
fwyzard@slc6:~$ file /proc/self/fd/2
/proc/self/fd/2: broken symbolic link to `/dev/pts/2'

@fwyzard
Copy link
Contributor Author

fwyzard commented Dec 1, 2016

By the way, does it make a difference if you use a non-privileged container ?

add a workaround for bash version 4.0 and older
@cmsbuild
Copy link

cmsbuild commented Dec 1, 2016

Pull request #78 was updated.

external issue cms-sw/cmsdist#2587

@fwyzard
Copy link
Contributor Author

fwyzard commented Dec 1, 2016

I've committed a workaround for older versions of BASH, can you give it a try ?

@davidlt
Copy link

davidlt commented Dec 1, 2016

I cannot do unprivileged container. Fedora does not package cgmanager, which is gateway for cgroup management from user perspective. I would need to build missing pieces myself.

@fwyzard
Copy link
Contributor Author

fwyzard commented Dec 1, 2016

I've tried, and I can write to /dev/stdout and /dev/stderr if I log in as root in a privileged container.

As a standard user in a privileged container, or as root in an unprivileged container, I cannot.

@fwyzard
Copy link
Contributor Author

fwyzard commented Dec 5, 2016

please test, comment, or merge ?

@smuzaffar smuzaffar merged commit 3259428 into cms-sw:master Dec 6, 2016
@fwyzard fwyzard deleted the no_file_descriptors branch April 4, 2018 10:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants