Skip to contentSkip to conversation

pseudo-wrapper: fix console issue

PATCH Superseded Gaël PORTAY 4 participants 8 messages Nov 23, 2016 – Nov 24, 2016

lore
Share & tools
lore
#0 lore raw

PATCH v3 1/1 pseudo-wrapper: fix console issue

Pseudo consists of:
* a client (cli),
* a server (daemon) and
* a database managed by this daemon
which makes a relation between inode, filename, type/mode...

Without -f/-d argument, the client forks a process, sets LD_PRELOAD, execve's the command given as argument and waits for its completion. The LD_PRELOAD'ed library tries to connect to the daemon and spawns it if it isn't running.

Neither the client itself nor the LD_PRELOAD'ed library waits for the daemon to terminate. As a consequence, the daemon may still be alive after the completion of the command executed through the client.

Under some circumstances (running build in a docker), the daemon (which may be still alive after the build) is killed with SIGKILL, thus it does not let it the time to properly sync its database.

The database is rendered inconsistent for the next build. In this case, makedevs will complain about the file type/mode, see error below.

makedevs: line xx: node (...)/output/target/dev/console exists but is of wrong type

Note that this error was introduced by c85cd189.

Because the database did not have the time to sync (ie. /dev/console record is missing), makedevs tests the existing and real /dev/console file mode (created by the previous build) which is a regular file to what should have been reported by pseudo, i.e. a character device. Those two modes are different and makedevs exits with error.

To solve this issue, this patch make the wrapper control the life of the daemon. It spawns the server before running the script inside the pseudo context, tells it to stop with "pseudo -S", and waits for the server to exit before the wrapper exits.

---
 package/pseudo/pseudo-wrapper | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)
package/pseudo/pseudo-wrapper +15 -2
index 9c8dbdb..194c88f 100644
@@ -8,7 +8,6 @@ if [ "${0##*/}" = "fakeroot" ]; then
 fi
 
 export PSEUDO_PREFIX="$(dirname "${0%/*}")"
-export PSEUDO_OPTS="-t0"
 if [ -n "${TARGET_DIR}" ]; then
     export PSEUDO_PASSWD="${TARGET_DIR}"
 fi
@@ -16,4 +15,18 @@ if [ -n "${BASE_DIR}" ]; then
     export PSEUDO_LOCALSTATEDIR="${BASE_DIR}/build/.pseudodb"
 fi
 
-exec "${0%/*}/pseudo" "${@}"
+# spawn server manually...
+"${0%/*}/pseudo" -f &
+pid="${!}"
+
+# ... run script/command...
+"${0%/*}/pseudo" "${@}"
+rc="${?}"
+
+# ... terminate server...
+"${0%/*}/pseudo" -S
+
+# ... and wait for its completion to make sure database is synced before
+# returning.
+wait "${pid}"
+exit "${rc}"
Signature · 1 line
2.10.2
#1 lore raw

PATCH v3 1/1 pseudo-wrapper: fix console issue

Ga?l, All,

On 2016-11-23 12:36 -0500, Ga?l PORTAY spake thusly:

Quoted 56 lines

Pseudo consists of:
* a client (cli),
* a server (daemon) and
* a database managed by this daemon
which makes a relation between inode, filename, type/mode...

Without -f/-d argument, the client forks a process, sets LD_PRELOAD, execve's the command given as argument and waits for its completion. The LD_PRELOAD'ed library tries to connect to the daemon and spawns it if it isn't running.

Neither the client itself nor the LD_PRELOAD'ed library waits for the daemon to terminate. As a consequence, the daemon may still be alive after the completion of the command executed through the client.

Under some circumstances (running build in a docker), the daemon (which may be still alive after the build) is killed with SIGKILL, thus it does not let it the time to properly sync its database.

The database is rendered inconsistent for the next build. In this case, makedevs will complain about the file type/mode, see error below.

makedevs: line xx: node (...)/output/target/dev/console exists but is of wrong type

Note that this error was introduced by c85cd189.

Because the database did not have the time to sync (ie. /dev/console record is missing), makedevs tests the existing and real /dev/console file mode (created by the previous build) which is a regular file to what should have been reported by pseudo, i.e. a character device. Those two modes are different and makedevs exits with error.

To solve this issue, this patch make the wrapper control the life of the daemon. It spawns the server before running the script inside the pseudo context, tells it to stop with "pseudo -S", and waits for the server to exit before the wrapper exits.

---
 package/pseudo/pseudo-wrapper | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)
package/pseudo/pseudo-wrapper +0 -1
index 9c8dbdb..194c88f 100644
@@ -8,7 +8,6 @@ if [ "${0##*/}" = "fakeroot" ]; then
 fi
 
 export PSEUDO_PREFIX="$(dirname "${0%/*}")"
-export PSEUDO_OPTS="-t0"
 if [ -n "${TARGET_DIR}" ]; then
     export PSEUDO_PASSWD="${TARGET_DIR}"
 fi

package/pseudo/pseudo-wrapper if [ -n "${BASE_DIR}" ]; then · line 20

Quoted 7 lines
package/pseudo/pseudo-wrapper +3 -1
@@ -16,4 +15,18 @@ if [ -n "${BASE_DIR}" ]; then
     export PSEUDO_LOCALSTATEDIR="${BASE_DIR}/build/.pseudodb"
 fi
 
-exec "${0%/*}/pseudo" "${@}"
+# spawn server manually...
+"${0%/*}/pseudo" -f &
+pid="${!}"

There still is a little corner case I'm not too happy about... The above returns immediately, but we don;t know if the daemon is ready. It may take a bit of time for it to start compeltely (i.e. reload the DB and repair its now-internal state)...

package/pseudo/pseudo-wrapper if [ -n "${BASE_DIR}" ]; then · line 23

Quoted 2 lines
package/pseudo/pseudo-wrapper +2 -0
+# ... run script/command...
+"${0%/*}/pseudo" "${@}"

But then we (almost) immediately start a client, which may try to connect to the daemon while it is not yet ready, and thus the client may not detect it yet, and thus wmay spawn his own daemon.

And we'd be back to square one...

Of course, we could run:

pseudo -d

which is guaranteed to return after the daemon is ready...

package/pseudo/pseudo-wrapper if [ -n "${BASE_DIR}" ]; then · line 31

Quoted 8 lines
package/pseudo/pseudo-wrapper +8 -0
+rc="${?}"
+
+# ... terminate server...
+"${0%/*}/pseudo" -S
+
+# ... and wait for its completion to make sure database is synced before
+# returning.
+wait "${pid}"

But then we would not have a PID to wait on... :-/

So, the question is: are we sure the client will talk to the daemon that we spawn?
If the answer is "yes, we're sure", then all is good; id the answer is "no, we're not sure", then it's a problem...

Thoughts?

Now, my position on all this mess is :

- we revert back to using fakeroot for 2016.11;

- long term, we want to definitely switch to using pseudo:
- it is actively maintained,,
- it has more features than fakeroot,
- some of those features are very interesting for us.

Regards,
Yann E. MORIN.

Quoted 4 lines
package/pseudo/pseudo-wrapper +1 -0
+exit "${rc}"
Signature · 1 line
2.10.2
Signature · 6 lines
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'
#2 lore raw

PATCH v3 1/1 pseudo-wrapper: fix console issue

Hello,

Quoted 4 lines · Yann E. MORIN

- long term, we want to definitely switch to using pseudo:
- it is actively maintained,,
- it has more features than fakeroot,
- some of those features are very interesting for us.

Could you explain which additional features of pseudo are interesting for us?

Thanks,

Thomas

Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
#3 lore raw

PATCH v3 1/1 pseudo-wrapper: fix console issue

Thomas, All,

On 2016-11-23 21:57 +0100, Thomas Petazzoni spake thusly:

Quoted 8 lines · Yann E. MORIN
Quoted 4 lines

- long term, we want to definitely switch to using pseudo:
- it is actively maintained,,
- it has more features than fakeroot,
- some of those features are very interesting for us.

Could you explain which additional features of pseudo are interesting for us?

pseudo can intercept and redirect acceses to /etc/paswd (and group, shadow, gshadow) to the ones in the target. With that, we could probably greatly simplify our handling of users.

Regards,
Yann E. MORIN.

Signature · 6 lines
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'
#4 lore raw

PATCH v3 1/1 pseudo-wrapper: fix console issue

Hello,

Quoted 7 lines · Yann E. MORIN

On 2016-11-23 21:57 +0100, Thomas Petazzoni spake thusly:

Quoted 6 lines · Yann E. MORIN
Quoted 4 lines

- long term, we want to definitely switch to using pseudo:
- it is actively maintained,,
- it has more features than fakeroot,
- some of those features are very interesting for us.

Quoted 6 lines
Quoted 2 lines

Could you explain which additional features of pseudo are interesting for us?

pseudo can intercept and redirect acceses to /etc/paswd (and group, shadow, gshadow) to the ones in the target. With that, we could probably greatly simplify our handling of users.

Is that the "additional features that are very interesting for us"? If it is, then I don't think it's that interesting. At least it definitely doesn't offset the drawback of having to build host-sqlite and host-attr.

Our handling of users is not complicated, it's very explicit, works fine, and is actually probably better than some black magic that intercepts accesses to /etc/passwd.

Thomas

Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
#5 lore raw

PATCH v3 1/1 pseudo-wrapper: fix console issue

In reply to Yann E. MORIN
Quoted 3 lines · Yann E. MORIN

Ga?l, All,

On 2016-11-23 12:36 -0500, Ga?l PORTAY spake thusly:

[snip]

Quoted 25 lines

package/pseudo/pseudo-wrapper if [ -n "${BASE_DIR}" ]; then · line 20

Quoted 4 lines
package/pseudo/pseudo-wrapper +3 -1
-exec "${0%/*}/pseudo" "${@}"
+# spawn server manually...
+"${0%/*}/pseudo" -f &
+pid="${!}"

There still is a little corner case I'm not too happy about... The above returns immediately, but we don;t know if the daemon is ready. It may take a bit of time for it to start compeltely (i.e. reload the DB and repair its now-internal state)...

Quoted 2 lines

+# ... run script/command...
+"${0%/*}/pseudo" "${@}"

But then we (almost) immediately start a client, which may try to connect to the daemon while it is not yet ready, and thus the client may not detect it yet, and thus wmay spawn his own daemon.

And we'd be back to square one...

Of course, we could run:

pseudo -d

which is guaranteed to return after the daemon is ready...

Quoted 10 lines

package/pseudo/pseudo-wrapper if [ -n "${BASE_DIR}" ]; then · line 31

Quoted 8 lines
package/pseudo/pseudo-wrapper +8 -0
+rc="${?}"
+
+# ... terminate server...
+"${0%/*}/pseudo" -S
+
+# ... and wait for its completion to make sure database is synced before
+# returning.
+wait "${pid}"

But then we would not have a PID to wait on... :-/

PID is not a problem, we can get it from LOCALSTATEDIR. The problem is that the
wait builtin only works for direct children of the shell.

Quoted 5 lines

So, the question is: are we sure the client will talk to the daemon that we spawn?
If the answer is "yes, we're sure", then all is good; id the answer is "no, we're not sure", then it's a problem...

I don't think we're really sure, but I do believe that pseudo protects against
this situation, i.e. when two servers start up concurrently, only one of them will really run. The problem is that the one that commits suicide may be the one that we start from the pseudo wrapper, so we end up wait'ing for the wrong pid.

Quoted 6 lines

Thoughts?

Now, my position on all this mess is :

- we revert back to using fakeroot for 2016.11;

Yeah, I think we need to do that. Too bad for the Fedora users.

Regards,
Arnout

Quoted 14 lines

- long term, we want to definitely switch to using pseudo:
- it is actively maintained,,
- it has more features than fakeroot,
- some of those features are very interesting for us.

Regards,
Yann E. MORIN.

Quoted 4 lines

+exit "${rc}"

Signature · 1 line
2.10.2
Signature · 6 lines
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF
#6 lore raw

PATCH v3 1/1 pseudo-wrapper: fix console issue

Hi all,

This patch *should* be abandonned.

The issue has been fixed and mainlined by the Seebs (the maintainer of pseudo), see commit d6eb2df3fe63b765f35d62332add4d0e4e9c6a39.

Now we can update the wrapper script to run:
exec "${0%/*}/pseudo" -S "${@}"

The -S argument tells server to shutdown and "waits" for the server to complete.

Seebs has picked up the solution proposed by Arnout. It uses the socket to determine when the server has terminated. The socket is closed right before exiting.

Quoted 11 lines · Arnout Vandecappelle
Quoted 6 lines

So, the question is: are we sure the client will talk to the daemon that we spawn?
If the answer is "yes, we're sure", then all is good; id the answer is "no, we're not sure", then it's a problem...

I don't think we're really sure, but I do believe that pseudo protects against
this situation, i.e. when two servers start up concurrently, only one of them will really run. The problem is that the one that commits suicide may be the one that we start from the pseudo wrapper, so we end up wait'ing for the wrong pid.

There is a lock, thus only a single instance of a server could run.

Quoted 8 lines
Quoted 6 lines

Thoughts?

Now, my position on all this mess is :

- we revert back to using fakeroot for 2016.11;

Yeah, I think we need to do that. Too bad for the Fedora users.

Agree. Even if this issue can be easily fixed, it remains the issue reported by Jerome.

Regards,
Gael

#7 lore raw

PATCH v3 1/1 pseudo-wrapper: fix console issue

Quoted 1 line · Ga?l PORTAY

This patch *should* be abandonned.

In such a situation, you should mark the patch as "Changes Requested" in
patchwork. You can do that yourself for your own patches.

I've now done that.

Regards,
Arnout

Signature · 6 lines
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF