Gentoo Websites Logo
Go to: Gentoo Home Documentation Forums Lists Bugs Planet Store Wiki Get Gentoo!
Bug 360013 - sys-apps/openrc-0.8.0 loops forever when init.d is called as non-root user and rc_parallel=yes
Summary: sys-apps/openrc-0.8.0 loops forever when init.d is called as non-root user an...
Status: RESOLVED FIXED
Alias: None
Product: Gentoo Hosted Projects
Classification: Unclassified
Component: OpenRC (show other bugs)
Hardware: All Linux
: Normal normal (vote)
Assignee: OpenRC Team
URL: http://git.overlays.gentoo.org/gitweb...
Whiteboard:
Keywords:
Depends on:
Blocks: 399185
  Show dependency tree
 
Reported: 2011-03-22 21:46 UTC by James Le Cuirot
Modified: 2012-02-09 10:08 UTC (History)
0 users

See Also:
Package list:
Runtime testing required: ---


Attachments
strace log from /etc/init.d/alsasound save as non-root (alsasound-save.txt,841.79 KB, text/plain)
2011-03-23 11:35 UTC, James Le Cuirot
Details
Include the newer variables in the list returned by rc_service_extra_commands. (0001-Include-the-newer-extra_commands-and-extra_started_c.patch,809 bytes, patch)
2011-04-08 14:24 UTC, James Le Cuirot
Details | Diff
Don't output a prefix for extra commands. (0002-Don-t-output-a-prefix-for-extra-commands-allowing-no.patch,1.62 KB, patch)
2011-04-08 14:24 UTC, James Le Cuirot
Details | Diff
Don't output a prefix for extra commands. (0002-Don-t-output-a-prefix-for-extra-commands-allowing-no.patch,1.72 KB, patch)
2011-04-08 17:04 UTC, James Le Cuirot
Details | Diff
Use the newer extra_commands and extra_started_commands variables in the list returned by rc_service_extra_commands. (0001-Use-the-newer-extra_commands-and-extra_started_comma.patch,1.22 KB, patch)
2011-04-19 20:25 UTC, James Le Cuirot
Details | Diff
Make rc_service_extra_commands return an empty list rather than a list containing a single empty string when there are no extra commands. (0003-Make-rc_service_extra_commands-return-an-empty-list-.patch,1.11 KB, patch)
2011-04-20 11:07 UTC, James Le Cuirot
Details | Diff
0001-Leave-the-lock-loop-in-case-of-errors.patch (0001-Leave-the-lock-loop-in-case-of-errors.patch,1.17 KB, patch)
2011-07-11 15:46 UTC, Christian Ruppert (idl0r)
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Le Cuirot gentoo-dev 2011-03-22 21:46:47 UTC
OpenRC tries to establish a lock on /lib/rc/init.d/prefix.lock when executing a command. Non-root users are seemingly allowed to at least try to execute "custom" commands (such as "save" in alsasound) but because they don't have write permission for prefix.lock, it simply loops forever. This doesn't happen for regular commands like "start" probably because it checks whether the user is root first and then aborts before trying to establish the lock.

The lock is located in write_prefix of src/rc/runscript.c. Looking at the commit that introduced it, it ensures that multiple processes don't write to the same tty at the same time and seems to be purely cosmetic.

One way to work around it is to put "if (geteuid() == 0)" around the loop. I've tried this and it works. Another way might be to ensure the file already exists so that the user doesn't have to create it. Am I right in thinking that the presence of the file is irrelevant as it is the lock that counts?

I don't think restricting these commands to root is an option as there are some legitimate use cases. In my case, I'd like to add a "console" command to access a server console through tmux but for all users of the games group, not just root.
Comment 1 Jeroen Roovers (RETIRED) gentoo-dev 2011-03-22 22:46:14 UTC
Please post your `emerge --info' output too.
Comment 2 James Le Cuirot gentoo-dev 2011-03-22 22:59:40 UTC
Sorry, I should have at least said what my OpenRC version was but I did check the git repository to make sure this hadn't already been fixed.


Portage 2.1.9.44 (default/linux/amd64/10.0, gcc-4.5.2, glibc-2.13-r1, 2.6.38-gentoo x86_64)
=================================================================
System uname: Linux-2.6.38-gentoo-x86_64-Intel-R-_Core-TM-_i7_CPU_K_875_@_2.93GHz-with-gentoo-2.0.1
Timestamp of tree: Tue, 22 Mar 2011 14:45:01 +0000
distcc 3.1 x86_64-pc-linux-gnu [disabled]
ccache version 3.1.4 [disabled]
app-shells/bash:     4.1_p9
dev-java/java-config: 2.1.11-r3
dev-lang/python:     2.7.1, 3.1.3
dev-util/ccache:     3.1.4
dev-util/cmake:      2.8.3-r1
sys-apps/baselayout: 2.0.1-r1
sys-apps/openrc:     0.8.0
sys-apps/sandbox:    2.4
sys-devel/autoconf:  2.13::<unknown repository>, 2.68
sys-devel/automake:  1.9.6-r3, 1.10.3, 1.11.1
sys-devel/binutils:  2.21
sys-devel/gcc:       3.4.6-r2, 4.5.2
sys-devel/gcc-config: 1.4.1
sys-devel/libtool:   2.4-r1
sys-devel/make:      3.82
virtual/os-headers:  2.6.36.1 (sys-kernel/linux-headers)
ACCEPT_KEYWORDS="amd64 ~amd64"
ACCEPT_LICENSE="*"
CBUILD="x86_64-pc-linux-gnu"
CFLAGS="-march=core2 -O2 -pipe"
CHOST="x86_64-pc-linux-gnu"
CONFIG_PROTECT="/etc /usr/share/gnupg/qualified.txt /var/lib/hsqldb"
CONFIG_PROTECT_MASK="/etc/ca-certificates.conf /etc/env.d /etc/env.d/java/ /etc/eselect/postgresql /etc/fonts/fonts.conf /etc/gconf /etc/gentoo-release /etc/php/apache2-php5.3/ext-active/ /etc/php/cgi-php5.3/ext-active/ /etc/php/cli-php5.3/ext-active/ /etc/revdep-rebuild /etc/sandbox.d /etc/splash /etc/terminfo /etc/texmf/language.dat.d /etc/texmf/language.def.d /etc/texmf/updmap.d /etc/texmf/web2c"
CXXFLAGS="-march=core2 -O2 -pipe"
DISTDIR="/usr/portage/distfiles"
FEATURES="assume-digests binpkg-logs distlocks fixlafiles fixpackages news parallel-fetch protect-owned sandbox sfperms strict unknown-features-warn unmerge-logs unmerge-orphans userfetch"
FFLAGS=""
GENTOO_MIRRORS="http://gentoo.virginmedia.com http://gentoo.osuosl.org"
INSTALL_MASK="/etc/asterisk /usr/lib/distcc/bin"
LANG="en_GB.UTF-8"
LC_ALL="en_GB.UTF-8"
LDFLAGS="-Wl,--as-needed"
LINGUAS="en en_GB"
MAKEOPTS="-j8"
PKGDIR="/usr/portage/packages"
PORTAGE_CONFIGROOT="/"
PORTAGE_RSYNC_OPTS="--recursive --links --safe-links --perms --times --compress --force --whole-file --delete --stats --timeout=180 --exclude=/distfiles --exclude=/local --exclude=/packages"
PORTAGE_TMPDIR="/var/tmp"
PORTDIR="/usr/portage"
PORTDIR_OVERLAY="/usr/local/portage/layman/java-overlay /usr/local/portage/layman/kde /usr/local/portage /home/chewi/chewi-overlay /usr/local/portage/layman/MythTV/Gentoo"
SYNC="rsync://rsync.europe.gentoo.org/gentoo-portage"
USE="16bit 3ds 7zip S3TC X X509 a52 aac aalib acpi aften aim alaw allegro amd amd64 amr amrnb amrwb asf async audiofile autoipd avahi bash-completion bcp berkdb bjam blender-game bluetooth bogofilter boost branding bzip2 cairo cap cardbus ccache cdaudio cdda cddb cdio cdparanoia cdr cdrom cdsound cegui cgi chardet cleartype cli community console consolekit cracklib crosscompile crypt cscope css ctype cups curl cxx dba dbus dc1394 device-mapper devil dhcp directfb divx dri dts dv dvb dvd dvdnav dvdr dvdread effects emoticon enca encode exceptions exif faac faad fam fastcgi fat fb fbcon fbcondecor fbdev fbsplash ffmpeg fftw filter firefox firefox3 flac fluidsynth fmod fontconfig ftp fuse g722 g729 gallium gd gdbm gdu geos gif gimp gimpprint gkrellm glitz glut gmp gnutls gphoto2 gpm gs gsl gsm gstreamer gtk guitarhero hash hddtemp hog icon iconv icq id3 id3tag ieee1394 ilbc image imagemagick imap imlib inkjar islsm_2.5.8.0 ithreads j2me jabber jack-tmpfs java5 java6 javascript jdbc4 jfs joystick jpeg json kdrive kvm ladcca ladspa lame libffi libnotify libsamplerate libv4l2 libvisual lights lighttpd lirc live lj lm_sensors lufsusermount lvm lzo m17n-lib mad maildir mbox mbrola md5sum mdnsresponder-compat mikmod ming mmap mmx mmxext mng mod mod_muc modplug modules mozbranding mozembed moznocompose moznoirc moznomail mozp3p mozsvg mp2 mp3 mp4 mpeg mpeg2 mpeg4 mplayer mpx msn mudflap multilib musepack music mvl mysql mysqli mythtv ncurses network nfs nfsv3 nfsv4 no-seamonkey nocd nosamples nptl nptlonly nsplugin ntfs offensive ogg openal opencore-amr opengl openmp openssl pam pcre pdf pg-intdatetime phonehome png pnm policykit posix postgres ppds pppd pulseaudio qmax qt3support qt4 quicktime rar readline realmedia reiserfs rtc rtsp ruby samba sasl scanner scenarios scrobbler sdl sdl-image sdl-sound sdlaudio secure-delete session sha512 simplexml skins slang smp sndfile soap sockets socks5 sound soundex sounds soundtouch sox speex spell sse sse2 sse3 sse4 sse4a ssl ssse3 startup-notification stemmer stream subversion svg svgz sysfs sysvipc taglib textures tga theora threads thumbnail thunar tiff timidity tordns transparent-proxy truetype type1 udev uk_rt ulaw unicode unzip upnp usb userlocales utf8 v4l v4l2 vcd vhosts videos vispatch vnc vncviewer vorbis wav web wideband wifi win32 win64 wma wmf wmp x264 xattr xcb xchattext xcomposite xext xface xfce xfs xft xml xmms2 xorg xosd xpm xprint xrandr xsl xulrunner xv xvid xvmc xvnc yahoo zeroconf zip zlib zsh-completion" ALSA_CARDS="intel-hda" ALSA_PCM_PLUGINS="adpcm alaw asym copy dmix dshare dsnoop empty extplug file hooks iec958 ioplug ladspa lfloat linear meter mmap_emul mulaw multi null plug rate route share shm softvol" APACHE2_MODULES="actions alias auth_basic authn_alias authn_anon authn_dbm authn_default authn_file authz_dbm authz_default authz_groupfile authz_host authz_owner authz_user autoindex cache cgi cgid dav dav_fs dav_lock deflate dir disk_cache env expires ext_filter file_cache filter headers include info log_config logio mem_cache mime mime_magic negotiation rewrite setenvif speling status unique_id userdir usertrack vhost_alias" CAMERAS="ptp2" COLLECTD_PLUGINS="df interface irq load memory rrdtool swap syslog" ELIBC="glibc" GPSD_PROTOCOLS="ashtech aivdm earthmate evermore fv18 garmin garmintxt gpsclock itrax mtk3301 nmea ntrip navcom oceanserver oldstyle oncore rtcm104v2 rtcm104v3 sirf superstar2 timing tsip tripmate tnt ubx" INPUT_DEVICES="evdev" KERNEL="linux" LCD_DEVICES="bayrad cfontz cfontz633 glk hd44780 lb216 lcdm001 mtxorb ncurses text" LINGUAS="en en_GB" LIRC_DEVICES="all" NGINX_MODULES_HTTP="access autoindex browser charset empty_gif fastcgi geo gzip limit_req limit_zone map proxy referer rewrite split_clients ssi upstream_ip_hash userid" PHP_TARGETS="php5-3" RUBY_TARGETS="ruby18" USERLAND="GNU" VIDEO_CARDS="radeon r600" XTABLES_ADDONS="quota2 psd pknock lscan length2 ipv4options ipset ipp2p iface geoip fuzzy condition tee tarpit sysrq steal rawnat logmark ipmark dhcpmac delude chaos account" 
Unset:  CPPFLAGS, CTARGET, EMERGE_DEFAULT_OPTS, PORTAGE_BUNZIP2_COMMAND, PORTAGE_COMPRESS, PORTAGE_COMPRESS_FLAGS, PORTAGE_RSYNC_EXTRA_OPTS
Comment 3 SpanKY gentoo-dev 2011-03-23 04:25:50 UTC
please post the exact command you're running.  `/etc/init.d/alsasound save` does not infinite loop on me with openrc-0.8.0.

also run said command through strace and post the log as an attachment:
strace -s 4096 -v -f -o log ...
Comment 4 James Le Cuirot gentoo-dev 2011-03-23 11:35:22 UTC
Created attachment 266981 [details]
strace log from /etc/init.d/alsasound save as non-root

"/etc/init.d/alsasound save" is the command I'm using. I've attached the log. I had to kill it almost immediately to stop it becoming huge.

The only unusual thing about my system that's partially relevant is that the default umask is 077. I say partially for several reasons. Most importantly, I've tried it on another system with the usual umask of 022 and it happens there too. Even on a system with umask 077, the system seems to boot up with umask 022 so when prefix.lock gets created on the tmpfs, it has mode 644. The file is never deleted but if I do delete it manually and restart some service as root, then yes, it does get recreated with mode 600 but a non-root user can't write to it in either case so this is not the source of the problem.

Having seen the code, I can't understand how it could possibly work for you. What are the permissions on your prefix.lock file?
Comment 5 SpanKY gentoo-dev 2011-04-07 05:27:06 UTC
the lock is only grabbed in parallel setups.  which i wasnt using but you apparently are.
Comment 6 James Le Cuirot gentoo-dev 2011-04-07 07:43:22 UTC
Correct, I'd forgotten about that setting.
Comment 7 SpanKY gentoo-dev 2011-04-07 19:47:59 UTC
this is kind of a pickle as to what the "correct" behavior is.  but i guess it doesnt make much sense to continue on if grabbing the lock fails with EPERM.  just issue a "you must be root" and forget about it.

certainly for the use case presented (calling "save"), this is OK.  and for common non-root options like "status", we dont need/grab the lock.
Comment 8 James Le Cuirot gentoo-dev 2011-04-07 21:02:41 UTC
Well, as I said, I would like to add a couple of commands to a script that would legitimately be run as non-root. Surely these, like status, would not require the lock. Even alsasound's save doesn't really need it.
Comment 9 SpanKY gentoo-dev 2011-04-07 21:25:43 UTC
"status" should be unaffected since it doesnt grab the lock.  as for alsa saving, i'm pretty sure you do need root.  it writes things out to /var/lib/alsa/ which is owned by root:root.

any other commands you will need to explicitly mention.
Comment 10 James Le Cuirot gentoo-dev 2011-04-08 14:23:08 UTC
alsasound's save, and also hwclock's save and show, do need you to be root but they don't need the lock. Strictly speaking, these commands should check whether you are root first. It shouldn't be assumed.

I want to add "console" and "command" commands to my minecraft-server script. I'm not sure why you're asking though as these cases obviously shouldn't be hardcoded into OpenRC. The $extra_commands and $extra_started_commands variables allow you to add any arbitrary commands to a script. Note that alsasound uses $opts instead, which is the old baselayout-1 syntax.

I've dug deeper into the source and come up with a couple of patches.

The first fixes another problem I found where librc's rc_service_extra_commands only returns the commands defined by $opts and not by the newer variables. Judging by sh/runscript.sh.in and the fact that $opts isn't documented anywhere, I suspect this wasn't intentional.

The second disables the prefixed output when executing one of these "extra" commands, therefore treating them in the same manner as describe, help and depend. It is this prefixed output that establishes the lock, hence the name prefix.lock. I believe this is safe to do as these commands are never executed in parallel as far as I'm aware.

Before:
# /etc/init.d/hwclock show
hwclock| Fri 08 Apr 2011 15:07:19 BST  -0.359917 seconds

After:
# /etc/init.d/hwclock show
Fri 08 Apr 2011 15:07:19 BST  -0.359917 seconds

This leaves the issue of the user needing to be root for some commands.

# /etc/init.d/alsasound save
 * Storing ALSA Mixer Levels ...
alsactl: save_state:1532: Cannot open /var/lib/alsa/asound.state for writing: Permission denied
 * Error saving levels.

As I said above, it should check but in this particular case, why not make asound.state writable for the audio group? ALSA allows such users to adjust the mixer normally. If this sounds good to you, I'll make the necessary patches for this and the other scripts that use $extra_commands.
Comment 11 James Le Cuirot gentoo-dev 2011-04-08 14:24:11 UTC
Created attachment 269017 [details, diff]
Include the newer variables in the list returned by rc_service_extra_commands.
Comment 12 James Le Cuirot gentoo-dev 2011-04-08 14:24:43 UTC
Created attachment 269019 [details, diff]
Don't output a prefix for extra commands.
Comment 13 James Le Cuirot gentoo-dev 2011-04-08 17:04:19 UTC
Created attachment 269049 [details, diff]
Don't output a prefix for extra commands.

Slight adjustment so that the extra_commands are only queried once.
Comment 14 James Le Cuirot gentoo-dev 2011-04-09 16:43:39 UTC
Hmmm there's a bigger problem. Some older scripts (like firehol) include ALL the commands in $opts, not just the extra ones. The second patch stops these from working. I'll have a think. :(
Comment 15 William Hubbs gentoo-dev 2011-04-09 20:26:49 UTC
(In reply to comment #14)
> Hmmm there's a bigger problem. Some older scripts (like firehol) include ALL
> the commands in $opts, not just the extra ones. The second patch stops these
> from working. I'll have a think. :(

When you say the second patch, do you mean the one in comment #12? If so, do they work ok with the one in comment #13?

If, on the other hand, you mean the patch in comment #11, we can use this, as is, just after we stabilize. Then we could open bugs against the affected packages and have them convert from using $opts to $extra_commands and $extra_started_commands.
Comment 16 James Le Cuirot gentoo-dev 2011-04-09 21:14:23 UTC
I meant #13. But don't take #11 either for now because fixing this may involve filtering out the usual commands (start, stop, restart) from the commands returned by rc_service_extra_commands.

Having looked at variable scripts, it seems that the precise definition of $opts was never really stated. Here are some examples...

alsasound: opts="save restore"
firehol: opts="start stop restart try status panic save"
sshd: opts="${opts} reload checkconfig gen_keys"

runscript.sh from baselayout-1 tells me that $opts had two purposes. One was to act as a filter for the functions in the script that could be executed as commands. The other was to display the available commands in the usage text. This is what happens on a baselayout-1 system.

# /etc/init.d/alsasound foobar
 * ERROR: wrong args ( foobar )
 * Usage: alsasound { save|restore }
 *        alsasound without arguments for full help

# /etc/init.d/firehol foobar
 * ERROR: wrong args ( foobar )
 * Usage: firehol { start|stop|restart|try|status|panic|save }
 *        firehol without arguments for full help

# /etc/init.d/sshd foobar
 * ERROR: wrong args ( foobar )
 * Usage: sshd { start|stop|restart|reload|checkconfig|gen_keys }
 *        sshd without arguments for full help

Not exactly consistent. It didn't matter that alsasound didn't include start, stop or restart because these are handled before $opts is checked.

Replacing everything with $extra_commands would break baselayout-1 - but I think you know that since you mention stabilising and I had heard that was on the cards. Otherwise, we'll have to filter the usual commands as I said above.
Comment 17 James Le Cuirot gentoo-dev 2011-04-19 20:25:05 UTC
Created attachment 270575 [details, diff]
Use the newer extra_commands and extra_started_commands variables in the list returned by rc_service_extra_commands.

To get my initial problem fixed before stabilisation, use this patch instead of #11. For rc_service_extra_commands, this replaces $opts altogether instead of treating it as a fallback. That way it'll only apply the new behaviour to newer scripts. Catering for $opts isn't important when you consider that it is going to disappear soon and rc_service_extra_commands has seemingly never been used anywhere before.
Comment 18 William Hubbs gentoo-dev 2011-04-19 23:55:02 UTC
Does this break backward compatibility in any way?

If so I am not comfortable adding it before we go stable.

Thanks,

William
Comment 19 James Le Cuirot gentoo-dev 2011-04-20 00:33:36 UTC
It will affect anything using rc_service_extra_commands but nothing actually seems to use this function and the current behaviour is technically wrong anyway. Old scripts using $opts will continue to behave in the same way. New scripts using $extra_commands will no longer be affected by this bug. So in short, no. :)

To clarify, I need you to apply both patches, #13 and #17.
Comment 20 James Le Cuirot gentoo-dev 2011-04-20 00:40:21 UTC
Sorry, I just spotted one more existing bug in rc_service_extra_commands that I hadn't noticed before. Just as well no one was using this function! ;) It's very late now though so I'll check it in the morning.
Comment 21 James Le Cuirot gentoo-dev 2011-04-20 11:07:02 UTC
Created attachment 270633 [details, diff]
Make rc_service_extra_commands return an empty list rather than a list containing a single empty string when there are no extra commands.

The problem wasn't as serious as I thought but I fixed it anyway. It looked as though rc_service_extra_commands would return NULL (indicating a problem) if there were no extra commands. Because of the way strsep works, it actually returned a list containing a single empty string, which wasn't really correct either, but it wasn't causing a problem. Now it returns an empty list as you might expect.

So please apply #13, #17 and then this one.
Comment 22 William Hubbs gentoo-dev 2011-04-28 04:38:18 UTC
Commit 04e256e is a modified version of the patch from comment #17. I
didn't see a reason to reaname the macro.

Commit 8fcaba9 is the patch from comment #20.

These will be part of openrc-0.8.3.

The patch from comment #13 has not been applied. Looking over the
discussion in comments 14-16, I'm not sure it can be applied in its
current form. From what you said in those comments it sounds like there
may still be issues with that patch.

Also, I would like to close this bug since we have several patches here
that are for different issues. Please remember that each issue should
have its own bug report.

Thanks much for your help and feel free to open another bug and attach
the new patch once you resolve the issues you referred to in comments
14-16.
Comment 23 James Le Cuirot gentoo-dev 2011-04-28 07:10:42 UTC
Thanks for applying those, although #13 is the one that ultimately fixes the issue I initially reported. The problems I mentioned in comments 14-16 are not an issue now that #17 has been applied. #13 can be safely applied as-is.
Comment 24 William Hubbs gentoo-dev 2011-04-28 13:54:14 UTC
I have a concern about #13 myself.
It is based on an assumption that all of the commands listed in
extra_commands will never be run in parallel. Is that always a safe
assumption?
Comment 25 James Le Cuirot gentoo-dev 2011-04-28 14:42:13 UTC
I believe so. These extra commands will only ever be called by the user, maybe as part of some external script but that wouldn't be in parallel. I can't think of any instance where one init script would call the extra command of another init script. How would it even do that, short of actually doing something like "/etc/init.d/alsasound save"? I don't think that's really allowed but if it did do that, the output would be handled by the calling script, which would already have a lock when acting in parallel. If the callee script also tried to obtain a lock then I think a deadlock would occur! Can you think of any other instances where an extra command may be called?
Comment 26 James Le Cuirot gentoo-dev 2011-05-06 09:03:11 UTC
Just saw the stabilisation announcement. Any chance of getting this one in given what I said above?
Comment 27 Christian Ruppert (idl0r) gentoo-dev 2011-07-11 15:46:54 UTC
Created attachment 279781 [details, diff]
0001-Leave-the-lock-loop-in-case-of-errors.patch
Comment 28 James Le Cuirot gentoo-dev 2011-07-11 21:52:20 UTC
I don't know if you intended that patch to be used as well as or instead of the one I already posted. I'm not sure if that code would ever be reached as non-root if my patch is applied but I guess it would be safest to check.
Comment 29 Christian Ruppert (idl0r) gentoo-dev 2011-07-11 22:13:58 UTC
(In reply to comment #28)
> I don't know if you intended that patch to be used as well as or instead of the
> one I already posted. I'm not sure if that code would ever be reached as
> non-root if my patch is applied but I guess it would be safest to check.

as well. We can probably drop the second eerror.. This is just to make sure we really leave the loop in case of errors even if it shouldn't be hit any more through your patches.

I also think that we should add a deprecation warning if "opts" is used and esp. if it also contains things such as "start", "stop".
Comment 30 Christian Ruppert (idl0r) gentoo-dev 2011-07-11 22:20:02 UTC
(In reply to comment #24)
> I have a concern about #13 myself.
> It is based on an assumption that all of the commands listed in
> extra_commands will never be run in parallel. Is that always a safe
> assumption?

It looks ok to me and I agree with #c25

Mike: What do you think?
Comment 31 James Le Cuirot gentoo-dev 2011-07-11 22:32:45 UTC
Actually I've just given the patch a proper look and now I'm not sure if it makes sense. If you're breaking on failure, it's not going to loop is it? But why were we looping in the first place when flock would block the call to open anyway?
Comment 32 James Le Cuirot gentoo-dev 2011-07-11 22:38:55 UTC
Wait, I'm a little sketchy on this stuff. It wouldn't block open, but it would block another call to flock, right? In any case, I don't think the loop is necessary.
Comment 33 Christian Ruppert (idl0r) gentoo-dev 2011-07-11 22:57:17 UTC
(In reply to comment #32)
> Wait, I'm a little sketchy on this stuff. It wouldn't block open, but it would
> block another call to flock, right? In any case, I don't think the loop is
> necessary.

I didn't read the complete function yet tbh but its AFAIK used to wait till a potential previous lock has been released.
Comment 34 James Le Cuirot gentoo-dev 2011-07-11 23:12:12 UTC
But that's what I'm saying, there's no need to wait with a loop because the call to flock would block in that case. On the other hand, it's theoretically possible that the call to flock might fail due to a signal interruption, in which case, you should probably try again. However, that case should be checked for specifically because flock may also result in a permanent failure.
Comment 35 Christian Ruppert (idl0r) gentoo-dev 2012-01-28 15:47:45 UTC
This has been "fixed" in Git.
There's more stuff to do to fix it properly but for now it doesn't loop anymore.

Also we should consider using (f)printf() instead, as it is already thread-safe through POSIX.
Comment 36 James Le Cuirot gentoo-dev 2012-01-28 16:17:49 UTC
Looks good. I tried it against 0.9.8.2 and it works. Agreed that fprintf is probably better if you're right about it being thread-safe.
Comment 37 Robin Johnson archtester Gentoo Infrastructure gentoo-dev Security 2012-02-09 09:41:59 UTC
It's not clear what was tested. Is this still a problem with 0.9.8.4 or current master?
Comment 38 James Le Cuirot gentoo-dev 2012-02-09 09:51:21 UTC
I meant I manually applied the patch to 0.9.8.2 and it fixed the problem. I haven't tried master recently. I can if you need me to but I trust Christian tested it sufficiently.
Comment 39 Robin Johnson archtester Gentoo Infrastructure gentoo-dev Security 2012-02-09 10:08:15 UTC
Ok, closing this for the tracker to show that we're close to a release.