[Patch v2] daemon: Set ownership of kept build directories to the calling user.

[Patch v2] daemon: Set ownership of kept build directories to the calling user.

[Hartmut Goebel]

Fixes <http://bugs.gnu.org/15890>.

* nix/libstore/globals.hh (Settings) Add clientUid and clientGid.
* nix/nix-daemon/nix-daemon.cc (pdaemonLoop] Store UID and GID of the
  caller in settings.
* nix/libstore/build.cc (_chown): New function.
  (DerivationGoal::deleteTmpDir): Use it, change ownership of build
  directory if it is kept.
---
 nix/libstore/build.cc        | 24 ++++++++++++++++++++++++
 nix/libstore/globals.hh      |  6 ++++++
 nix/nix-daemon/nix-daemon.cc | 13 +++++++++++++
 3 files changed, 43 insertions(+)

diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc
index ae78e65..b49fb95 100644
--- a/nix/libstore/build.cc
+++ b/nix/libstore/build.cc
@@ -2609,6 +2609,23 @@ void DerivationGoal::closeLogFile()
 }
 
 
+static void _chown(const Path & path, uid_t uid, gid_t gid)
+{
+    checkInterrupt();
+
+    printMsg(lvlVomit, format("%1%") % path);
+
+    if (chown(path.c_str(), uid, gid) == -1) {
+	throw SysError(format("change owner and group of `%1%'") % path);
+    }
+    struct stat st = lstat(path);
+    if (S_ISDIR(st.st_mode)) {
+        for (auto & i : readDirectory(path))
+            _chown(path + "/" + i.name, uid, gid);
+    }
+}
+
+
 void DerivationGoal::deleteTmpDir(bool force)
 {
     if (tmpDir != "") {
@@ -2617,6 +2634,13 @@ void DerivationGoal::deleteTmpDir(bool force)
                 format("note: keeping build directory `%2%'")
                 % drvPath % tmpDir);
             chmod(tmpDir.c_str(), 0755);
+            // Change the ownership if clientUid is set. Never change the
+            // ownership to "root" for security reasons. So zero is used as
+            // marker for unset.
+            if (settings.clientUid != 0) {
+                _chown(tmpDir, settings.clientUid,
+                       settings.clientGid != 0 ? settings.clientGid : -1);
+            }
         }
         else
             deletePath(tmpDir);
diff --git a/nix/libstore/globals.hh b/nix/libstore/globals.hh
index 8c07e36..dc6a004 100644
--- a/nix/libstore/globals.hh
+++ b/nix/libstore/globals.hh
@@ -70,6 +70,12 @@ struct Settings {
        subgoal of the same goal) fails. */
     bool keepGoing;
 
+    /* User and groud id of the client issuing the buld request.  Used to set
+       the owner and group of the keept temporary directories of failed
+       builds. */
+    uid_t clientUid;
+    gid_t clientGid;
+
     /* Whether, if we cannot realise the known closure corresponding
        to a derivation, we should try to normalise the derivation
        instead. */
diff --git a/nix/nix-daemon/nix-daemon.cc b/nix/nix-daemon/nix-daemon.cc
index 35c284f..e900a7d 100644
--- a/nix/nix-daemon/nix-daemon.cc
+++ b/nix/nix-daemon/nix-daemon.cc
@@ -950,6 +950,19 @@ static void daemonLoop()
                     strncpy(argvSaved[1], processName.c_str(), strlen(argvSaved[1]));
                 }
 
+#if defined(SO_PEERCRED)
+                /* Store the client's user and group for this connection. This
+                   has to be done in the forked process since it is per
+                   connection. */
+                settings.clientUid = cred.uid;
+                settings.clientGid = cred.gid;
+#else
+                /* Setting these to zero means: do not change, esp. do not
+                   change to "root". */
+                settings.clientUid = 0;
+                settings.clientGid = 0;
+#endif
+
                 /* Handle the connection. */
                 from.fd = remote;
                 to.fd = remote;
-- 
2.7.4

Re: [Patch v2] daemon: Set ownership of kept build directories to the calling user.

[Ludovic Courtès]

Hartmut Goebel <h.goebel@crazy-compilers.com> skribis:

> Fixes <http://bugs.gnu.org/15890>.
>
> * nix/libstore/globals.hh (Settings) Add clientUid and clientGid.
> * nix/nix-daemon/nix-daemon.cc (pdaemonLoop] Store UID and GID of the
>   caller in settings.
> * nix/libstore/build.cc (_chown): New function.
>   (DerivationGoal::deleteTmpDir): Use it, change ownership of build
>   directory if it is kept.

[...]

> +static void _chown(const Path & path, uid_t uid, gid_t gid)
> +{
> +    checkInterrupt();
> +
> +    printMsg(lvlVomit, format("%1%") % path);
> +
> +    if (chown(path.c_str(), uid, gid) == -1) {

I think this should use ‘lchown’.

> --- a/nix/libstore/globals.hh
> +++ b/nix/libstore/globals.hh
> @@ -70,6 +70,12 @@ struct Settings {
>         subgoal of the same goal) fails. */
>      bool keepGoing;
>  
> +    /* User and groud id of the client issuing the buld request.  Used to set
> +       the owner and group of the keept temporary directories of failed
> +       builds. */
> +    uid_t clientUid;
> +    gid_t clientGid;

I don’t like the idea of passing those via the big ‘Settings’
singleton.

Could we instead pass them via the ‘LocalStore’ constructor, with their
default values taken from ‘getuid’ and ‘getgid’ (rather than 0)?  WDYT?

Thank you!

Ludo’.

Re: [Patch v2] daemon: Set ownership of kept build directories to the calling user.

[Hartmut Goebel]

Am 21.11.2016 um 15:13 schrieb Ludovic Courtès:
> I don’t like the idea of passing those via the big ‘Settings’
> singleton.

Well, the Settings are already used to pass options like --keep-going to
the build process. So the "Singleton" is mutable per-process anyway.
This is why I've put these here, too.

But if you prefer passing them around, I'll try to implement this.

-- 
Regards
Hartmut Goebel

| Hartmut Goebel          | h.goebel@crazy-compilers.com               |
| www.crazy-compilers.com | compilers which you thought are impossible |

Re: [Patch v2] daemon: Set ownership of kept build directories to the calling user.

[Ludovic Courtès]

Hi!

Hartmut Goebel <h.goebel@crazy-compilers.com> skribis:

> Am 21.11.2016 um 15:13 schrieb Ludovic Courtès:
>> I don’t like the idea of passing those via the big ‘Settings’
>> singleton.
>
> Well, the Settings are already used to pass options like --keep-going to
> the build process. So the "Singleton" is mutable per-process anyway.
> This is why I've put these here, too.

Good point.  This and your other message have convinced me!

> Am 21.11.2016 um 15:13 schrieb Ludovic Courtès:
>> Could we instead pass them via the ‘LocalStore’ constructor, with their
>> default values taken from ‘getuid’ and ‘getgid’ (rather than 0)?  WDYT?
>
> Regarding the default values: I'm using zero as "not set" marker here.
> The files will already have owner and group from ‘getuid’ and ‘getgid’,
> so there is no need to change them. Or did I miss something?

The problem is that 0 also means “root”, so I’d prefer a different value
to denote that “unset” status, -1 if you want, and explicit checks (and
a comment explaining that ‘clientUid’ and ‘clientGid’ can be either a
valid UID/GID or -1, in which case blablabla).

How does that sound?

Thanks, and sorry for the delay!

Ludo’.

Re: [Patch v2] daemon: Set ownership of kept build directories to the calling user.

[Hartmut Goebel]

Am 27.11.2016 um 22:04 schrieb Ludovic Courtès:
> The problem is that 0 also means “root”, so I’d prefer a different value
> to denote that “unset” status, -1 if you want, and explicit checks (and
> a comment explaining that ‘clientUid’ and ‘clientGid’ can be either a
> valid UID/GID or -1, in which case blablabla).
>
> How does that sound?

This basically sounds reasonable.

I just need some C programming hint: uid_t is an unsigned int, so
comparing with -1 raises a warning (which IMO is the same as en error).
And casting uid_t to unisgned int might return the same uid as "nobody".
How can I solve this?

-- 
Regards
Hartmut Goebel

| Hartmut Goebel          | h.goebel@crazy-compilers.com               |
| www.crazy-compilers.com | compilers which you thought are impossible |

Re: [Patch v2] daemon: Set ownership of kept build directories to the calling user.

[Danny Milosavljevic]

Hi Hartmut,

On Mon, 28 Nov 2016 22:31:44 +0100
Hartmut Goebel <h.goebel@crazy-compilers.com> wrote:
> I just need some C programming hint: uid_t is an unsigned int, so
> comparing with -1 raises a warning (which IMO is the same as en error).

The system call handler in the Linux kernel does this, among other things:

#define low2highuid(uid) ((uid) == (old_uid_t)-1 ? (uid_t)-1 : (uid_t)(uid))

So I'd say to compare with (uid_t)-1 would be fine

    if (settings.clientUid != (uid_t) -1) {
        ...
    }

... since they do something like it in Linux anyway - which is the one implementing the chown operation in the first place :)

> And casting uid_t to unisgned int might return the same uid as "nobody".

What do you mean? uid_t is __uid_t - which in turn is implementation-defined - but yes, it is unsigned. On Linux it changed size multiple time until now where it is 32 bit unsigned.

On GuixSD, user "nobody" has uid 65534. That's one less than the maximum value for 16 bit (!) uids (the maximum is hopefully invalid as uid since chown needs it as a flag).

(-1) in two-complement 16 bit integral encoding (which is technically not guaranteed to be used in C) would be 65535.

[PATCH v3] daemon: Set ownership of kept build directories to the calling user.

[Hartmut Goebel]

Fixes <http://bugs.gnu.org/15890>.

* nix/libstore/globals.hh (Settings) Add clientUid and clientGid.
* nix/nix-daemon/nix-daemon.cc (daemonLoop] Store UID and GID of the
  caller in settings.
* nix/libstore/build.cc (_chown): New function.
  (DerivationGoal::deleteTmpDir): Use it, change ownership of build
  directory if it is kept and the new owner is not root.
---
 nix/libstore/build.cc        | 23 +++++++++++++++++++++++
 nix/libstore/globals.hh      |  6 ++++++
 nix/nix-daemon/nix-daemon.cc | 12 ++++++++++++
 3 files changed, 41 insertions(+)

diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc
index 889ee3d..44ff11c 100644
--- a/nix/libstore/build.cc
+++ b/nix/libstore/build.cc
@@ -2631,6 +2631,23 @@ void DerivationGoal::closeLogFile()
 }
 
 
+static void _chown(const Path & path, uid_t uid, gid_t gid)
+{
+    checkInterrupt();
+
+    printMsg(lvlVomit, format("%1%") % path);
+
+    if (lchown(path.c_str(), uid, gid) == -1) {
+	throw SysError(format("change owner and group of `%1%'") % path);
+    }
+    struct stat st = lstat(path);
+    if (S_ISDIR(st.st_mode)) {
+        for (auto & i : readDirectory(path))
+            _chown(path + "/" + i.name, uid, gid);
+    }
+}
+
+
 void DerivationGoal::deleteTmpDir(bool force)
 {
     if (tmpDir != "") {
@@ -2639,6 +2656,12 @@ void DerivationGoal::deleteTmpDir(bool force)
                 format("note: keeping build directory `%2%'")
                 % drvPath % tmpDir);
             chmod(tmpDir.c_str(), 0755);
+            // Change the ownership if clientUid is set. Never change the
+            // ownership or the group to "root" for security reasons.
+            if (settings.clientUid != (uid_t) -1 && settings.clientUid != 0) {
+                _chown(tmpDir, settings.clientUid,
+                       settings.clientGid != 0 ? settings.clientGid : -1);
+            }
         }
         else
             deletePath(tmpDir);
diff --git a/nix/libstore/globals.hh b/nix/libstore/globals.hh
index 8c07e36..dc6a004 100644
--- a/nix/libstore/globals.hh
+++ b/nix/libstore/globals.hh
@@ -70,6 +70,12 @@ struct Settings {
        subgoal of the same goal) fails. */
     bool keepGoing;
 
+    /* User and groud id of the client issuing the buld request.  Used to set
+       the owner and group of the keept temporary directories of failed
+       builds. */
+    uid_t clientUid;
+    gid_t clientGid;
+
     /* Whether, if we cannot realise the known closure corresponding
        to a derivation, we should try to normalise the derivation
        instead. */
diff --git a/nix/nix-daemon/nix-daemon.cc b/nix/nix-daemon/nix-daemon.cc
index 682f9a2..6f88e5b 100644
--- a/nix/nix-daemon/nix-daemon.cc
+++ b/nix/nix-daemon/nix-daemon.cc
@@ -960,6 +960,18 @@ static void daemonLoop()
                     strncpy(argvSaved[1], processName.c_str(), strlen(argvSaved[1]));
                 }
 
+#if defined(SO_PEERCRED)
+                /* Store the client's user and group for this connection. This
+                   has to be done in the forked process since it is per
+                   connection. */
+                settings.clientUid = cred.uid;
+                settings.clientGid = cred.gid;
+#else
+                /* Setting these to -1 means: do not change */
+                settings.clientUid = -1;
+                settings.clientGid = -1;
+#endif
+
                 /* Handle the connection. */
                 from.fd = remote;
                 to.fd = remote;
-- 
2.7.4

Re: [PATCH v3] daemon: Set ownership of kept build directories to the calling user.

[Ludovic Courtès]

Hartmut Goebel <h.goebel@crazy-compilers.com> skribis:

> Fixes <http://bugs.gnu.org/15890>.
>
> * nix/libstore/globals.hh (Settings) Add clientUid and clientGid.
> * nix/nix-daemon/nix-daemon.cc (daemonLoop] Store UID and GID of the
>   caller in settings.
> * nix/libstore/build.cc (_chown): New function.
>   (DerivationGoal::deleteTmpDir): Use it, change ownership of build
>   directory if it is kept and the new owner is not root.

[...]

> +static void _chown(const Path & path, uid_t uid, gid_t gid)
> +{
> +    checkInterrupt();
> +
> +    printMsg(lvlVomit, format("%1%") % path);

Please remove this line (we wouldn’t be able to track where the message
comes from).

> +    /* User and groud id of the client issuing the buld request.  Used to set
                                                       ^^
Typo.  Also:

  … issuing the build request, or -1 if the UID and GID are not known.

> +       the owner and group of the keept temporary directories of failed
                                      ^^
Typo.

If you have checked that it works as intended, please push with these
changes and email the commit ID to 15890-done@debbugs.gnu.org.

Thank you!

Ludo’.

Re: [PATCH v3] daemon: Set ownership of kept build directories to the calling user.

[Hartmut Goebel]

Am 06.12.2016 um 16:08 schrieb Ludovic Courtès:
> If you have checked that it works as intended, please push with these
> changes and email the commit ID to 15890-done@debbugs.gnu.org.

I can only verify that it worked with revision 6634180 "gnu: guile-ssh:
Update to 0.10.2.", but not with current master.

Starting with 21531ad "offload: Use Guile-SSH instead of GNU lsh" I get
errors, which I could not work around:

guix: offload: command not found
Try `guix --help' for more information.
guix build: error: build failed: unexpected EOF reading a line

So, although I'm confident the code is correct, I can not verify with
current master. Thus I'm posting a new version of the patch asking you
to test and push.

-- 
Regards
Hartmut Goebel

| Hartmut Goebel          | h.goebel@crazy-compilers.com               |
| www.crazy-compilers.com | compilers which you thought are impossible |

[PATCH v4] daemon: Set ownership of kept build directories to the calling user.

[Hartmut Goebel]

Fixes <http://bugs.gnu.org/15890>.

* nix/libstore/globals.hh (Settings) Add clientUid and clientGid.
* nix/nix-daemon/nix-daemon.cc (daemonLoop] Store UID and GID of the
  caller in settings.
* nix/libstore/build.cc (_chown): New function.
  (DerivationGoal::deleteTmpDir): Use it, change ownership of build
  directory if it is kept and the new owner is not root.
---
 nix/libstore/build.cc        | 21 +++++++++++++++++++++
 nix/libstore/globals.hh      |  6 ++++++
 nix/nix-daemon/nix-daemon.cc | 12 ++++++++++++
 3 files changed, 39 insertions(+)

diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc
index 889ee3d..e823001 100644
--- a/nix/libstore/build.cc
+++ b/nix/libstore/build.cc
@@ -2631,6 +2631,21 @@ void DerivationGoal::closeLogFile()
 }
 
 
+static void _chown(const Path & path, uid_t uid, gid_t gid)
+{
+    checkInterrupt();
+
+    if (lchown(path.c_str(), uid, gid) == -1) {
+	throw SysError(format("change owner and group of `%1%'") % path);
+    }
+    struct stat st = lstat(path);
+    if (S_ISDIR(st.st_mode)) {
+        for (auto & i : readDirectory(path))
+            _chown(path + "/" + i.name, uid, gid);
+    }
+}
+
+
 void DerivationGoal::deleteTmpDir(bool force)
 {
     if (tmpDir != "") {
@@ -2639,6 +2654,12 @@ void DerivationGoal::deleteTmpDir(bool force)
                 format("note: keeping build directory `%2%'")
                 % drvPath % tmpDir);
             chmod(tmpDir.c_str(), 0755);
+            // Change the ownership if clientUid is set. Never change the
+            // ownership or the group to "root" for security reasons.
+            if (settings.clientUid != (uid_t) -1 && settings.clientUid != 0) {
+                _chown(tmpDir, settings.clientUid,
+                       settings.clientGid != 0 ? settings.clientGid : -1);
+            }
         }
         else
             deletePath(tmpDir);
diff --git a/nix/libstore/globals.hh b/nix/libstore/globals.hh
index 8c07e36..7beb1a5 100644
--- a/nix/libstore/globals.hh
+++ b/nix/libstore/globals.hh
@@ -70,6 +70,12 @@ struct Settings {
        subgoal of the same goal) fails. */
     bool keepGoing;
 
+    /* User and groud id of the client issuing the build request.  Used to set
+       the owner and group of the kept temporary directories of failed
+       builds. */
+    uid_t clientUid;
+    gid_t clientGid;
+
     /* Whether, if we cannot realise the known closure corresponding
        to a derivation, we should try to normalise the derivation
        instead. */
diff --git a/nix/nix-daemon/nix-daemon.cc b/nix/nix-daemon/nix-daemon.cc
index 682f9a2..47b67d5 100644
--- a/nix/nix-daemon/nix-daemon.cc
+++ b/nix/nix-daemon/nix-daemon.cc
@@ -960,6 +960,18 @@ static void daemonLoop()
                     strncpy(argvSaved[1], processName.c_str(), strlen(argvSaved[1]));
                 }
 
+#if defined(SO_PEERCRED)
+                /* Store the client's user and group for this connection. This
+                   has to be done in the forked process since it is per
+                   connection. */
+                settings.clientUid = cred.uid;
+                settings.clientGid = cred.gid;
+#else
+                /* Setting these to -1 means: do not change */
+                settings.clientUid = (uid_t) -1;
+                settings.clientGid = (gid_t) -1;
+#endif
+
                 /* Handle the connection. */
                 from.fd = remote;
                 to.fd = remote;
-- 
2.7.4

Re: [PATCH v4] daemon: Set ownership of kept build directories to the calling user.

[Ludovic Courtès]

Hartmut Goebel <h.goebel@crazy-compilers.com> skribis:

> Fixes <http://bugs.gnu.org/15890>.
>
> * nix/libstore/globals.hh (Settings) Add clientUid and clientGid.
> * nix/nix-daemon/nix-daemon.cc (daemonLoop] Store UID and GID of the
>   caller in settings.
> * nix/libstore/build.cc (_chown): New function.
>   (DerivationGoal::deleteTmpDir): Use it, change ownership of build
>   directory if it is kept and the new owner is not root.

OK, please push.

Thank you!

Ludo’.

Re: [PATCH v4] daemon: Set ownership of kept build directories to the calling user.

[Hartmut Goebel]

Am 09.12.2016 um 15:23 schrieb Ludovic Courtès:
> OK, please push.
Done. Thanks for testing.

-- 
Regards
Hartmut Goebel

| Hartmut Goebel          | h.goebel@crazy-compilers.com               |
| www.crazy-compilers.com | compilers which you thought are impossible |

Re: [PATCH v3] daemon: Set ownership of kept build directories to the calling user.

[Ludovic Courtès]

Hartmut Goebel <h.goebel@crazy-compilers.com> skribis:

> Am 06.12.2016 um 16:08 schrieb Ludovic Courtès:
>> If you have checked that it works as intended, please push with these
>> changes and email the commit ID to 15890-done@debbugs.gnu.org.
>
> I can only verify that it worked with revision 6634180 "gnu: guile-ssh:
> Update to 0.10.2.", but not with current master.
>
> Starting with 21531ad "offload: Use Guile-SSH instead of GNU lsh" I get
> errors, which I could not work around:
>
> guix: offload: command not found
> Try `guix --help' for more information.
> guix build: error: build failed: unexpected EOF reading a line

You need to install Guile-SSH to fix this (Guile-SSH was found at
configure-time but not at run-time, leading to this error.)

Ludo’.

Guile-SSH found at configure-time but not at run-time

[Hartmut Goebel]

Am 09.12.2016 um 15:22 schrieb Ludovic Courtès:
> You need to install Guile-SSH to fix this (Guile-SSH was found at
> configure-time but not at run-time, leading to this error.)

I wonder how this can happen. I'm running within "guix environment guix".

Maybe I need to run within "./pre-inst guix environment guix"?

-- 
Regards
Hartmut Goebel

| Hartmut Goebel          | h.goebel@crazy-compilers.com               |
| www.crazy-compilers.com | compilers which you thought are impossible |

Re: Guile-SSH found at configure-time but not at run-time

[Ludovic Courtès]

Hartmut Goebel <h.goebel@crazy-compilers.com> skribis:

> Am 09.12.2016 um 15:22 schrieb Ludovic Courtès:
>> You need to install Guile-SSH to fix this (Guile-SSH was found at
>> configure-time but not at run-time, leading to this error.)
>
> I wonder how this can happen. I'm running within "guix environment guix".

In ‘guix environment guix’, Guile-SSH is definitely going to be found;
you can confirm this by looking at ‘config.log’.

However, if you type “make install” from there, then it may be the case
that Guile-SSH is no longer visible in the installed environment.

HTH!

Ludo’.

Re: [PATCH v3] daemon: Set ownership of kept build directories to the calling user.

[Danny Milosavljevic]

Hi Hartmut,

> +#if defined(SO_PEERCRED)
...
> +#else
> +                /* Setting these to -1 means: do not change */
> +                settings.clientUid = -1;
> +                settings.clientGid = -1;
> +#endif

I think you also have to cast them there, so

               settings.clientUid = (uid_t) -1;
               settings.clientGid = (gid_t) -1;

The reason is because (-1) is a signed integer and clientUid isn't - and neither is clientGid.

Re: [PATCH v3] daemon: Set ownership of kept build directories to the calling user.

[Hartmut Goebel]

Am 06.12.2016 um 21:41 schrieb Danny Milosavljevic:
> I think you also have to cast them there, so
>
>                settings.clientUid = (uid_t) -1;
>                settings.clientGid = (gid_t) -1;

Thanks, I changes this accordingly.

-- 
Regards
Hartmut Goebel

| Hartmut Goebel          | h.goebel@crazy-compilers.com               |
| www.crazy-compilers.com | compilers which you thought are impossible |

Re: [Patch v2] daemon: Set ownership of kept build directories to the calling user.

[Hartmut Goebel]

Am 01.12.2016 um 01:01 schrieb Danny Milosavljevic:
> So I'd say to compare with (uid_t)-1 would be fine
>
>     if (settings.clientUid != (uid_t) -1) {
>         ...
>     }

Thanks for this tip, Danny. This made it work and I just posted the
updated patch.

> > And casting uid_t to unisgned int might return the same uid as "nobody".
> [...]
>
> (-1) in two-complement 16 bit integral encoding (which is technically not guaranteed to be used in C) would be 65535.

Ahh, well, yes. I'm not quite good on bit operations, esp. on
representations of negative numbers. Thanks to doing this right.

-- 
Regards
Hartmut Goebel

| Hartmut Goebel          | h.goebel@crazy-compilers.com               |
| www.crazy-compilers.com | compilers which you thought are impossible |

Re: [Patch v2] daemon: Set ownership of kept build directories to the calling user.

[Hartmut Goebel]

Am 21.11.2016 um 15:13 schrieb Ludovic Courtès:
> Could we instead pass them via the ‘LocalStore’ constructor, with their
> default values taken from ‘getuid’ and ‘getgid’ (rather than 0)?  WDYT?

Regarding the default values: I'm using zero as "not set" marker here.
The files will already have owner and group from ‘getuid’ and ‘getgid’,
so there is no need to change them. Or did I miss something?

-- 
Regards
Hartmut Goebel

| Hartmut Goebel          | h.goebel@crazy-compilers.com               |
| www.crazy-compilers.com | compilers which you thought are impossible |

Re: [Patch v2] daemon: Set ownership of kept build directories to the calling user.

[Hartmut Goebel]

Am 21.11.2016 um 15:13 schrieb Ludovic Courtès:
> Could we instead pass them via the ‘LocalStore’ constructor,

I started implementing this, but adding clientUid/Gid as a attribute
(property) of 'LocalStore' feels wrong. The LocalStore is about the
store: files being there, querying and such. Even "build" is a bit of a
misfit IMO.

Okay, probably you mean, passing to the ‘LocalStore’ constructor and
then forward them to the goals. Which means changing
`makeDerevationGoal', which is used quite some time. From there you
would need to pass it onto the 'DerivationGoal' constructor. These are
quite some changes for the rare case of building, and only if the build
failed and only if the user passed --keep-failed.

I have the impression, that 'Settings' have been introduced exactly to
save passing around such information. "keep-failed" is passed to
'deleteTmpDir()' exactly this way (via Settings). Which means if passing
clientUid/Gid via constructors,  we would have two different ways of
passing information used in the same small function - which would be
inconsistent.

Thus said, I'm in strong favour of using Settings.

-- 
Regards
Hartmut Goebel

| Hartmut Goebel          | h.goebel@crazy-compilers.com               |
| www.crazy-compilers.com | compilers which you thought are impossible |