Hello Roel,
Roel Janssen <r...@gnu.org> skribis:
> Roel Janssen <r...@gnu.org> writes:
>
>> Ludovic Courtès <ludovic.cour...@inria.fr> writes:
[...]
>>> In this case, I thought guix-daemon could explicitly check whether the
>>> peer is remote, and disable GC in that case. That is, ‘guix gc’ would
>>> still work locally on the machine that runs guix-daemon, but it would no
>>> longer work remotely.
>>>
>>> How does that sound?
>>
>> That sounds like it solves our use-case, but only because in our
>> case the access to the machine running guix-daemon is limited.
>>
>> So, even though I'm not sure how to implement this, your solution is
>> fine with me.
>
> I implemented the solution in the attached patch. When a connection
> does not come from the UNIX socket, it is treated as “remote”. So,
> local TCP connections would also be treated as “remote”.
Excellent!
> I assumed ‘collectGarbage()’ is the entry point for all garbage collection,
> is that correct?
Yes.
> From 00f489d6303720c65571fdf0bc9ee810a20f70e0 Mon Sep 17 00:00:00 2001
> From: Roel Janssen <r...@gnu.org>
> Date: Wed, 11 Apr 2018 09:52:11 +0200
> Subject: [PATCH] guix-daemon: Disable garbage collection for remote hosts.
>
> * nix/libstore/gc.cc (collectGarbage): Check for remote connections.
> * nix/libstore/globals.hh: Add isRemoteConnection setting.
> * nix/nix-daemon/nix-daemon.cc (performOp): Display appropriate error message;
> (acceptConnection): Set isRemoteConnection when connection is over TCP.
[...]
> --- a/nix/libstore/gc.cc
> +++ b/nix/libstore/gc.cc
> @@ -595,6 +595,10 @@ void LocalStore::removeUnusedLinks(const GCState & state)
>
> void LocalStore::collectGarbage(const GCOptions & options, GCResults &
> results)
> {
> + if (settings.isRemoteConnection) {
> + return;
> + }
I think this is unnecessary since the daemon already checks for that.
(It’s also nicer to keep ‘LocalStore’ unaware of the connection
details.)
> diff --git a/nix/nix-daemon/nix-daemon.cc b/nix/nix-daemon/nix-daemon.cc
> index deb7003d7..65770ba95 100644
> --- a/nix/nix-daemon/nix-daemon.cc
> +++ b/nix/nix-daemon/nix-daemon.cc
> @@ -529,6 +529,11 @@ static void performOp(bool trusted, unsigned int
> clientVersion,
> }
>
> case wopCollectGarbage: {
> + if (settings.isRemoteConnection) {
> + throw Error("Garbage collection is disabled for remote hosts.");
> + break;
> + }
> GCOptions options;
> options.action = (GCOptions::GCAction) readInt(from);
> options.pathsToDelete = readStorePaths<PathSet>(from);
I was wondering if we would like to allow some of the ‘GCAction’ values,
but maybe it’s better to disallow them altogether like this code does.
> @@ -934,6 +939,7 @@ static void acceptConnection(int fdSocket)
> connection. Setting these to -1 means: do not change. */
> settings.clientUid = clientUid;
> settings.clientGid = clientGid;
> + settings.isRemoteConnection = (remoteAddr.ss_family !=
> AF_UNIX);
I think you can make ‘isRemoteConnection’ a static global variable in
nix-daemon.cc instead of adding it to ‘Settings’. So it would do
something like:
--8<---------------cut here---------------start------------->8---
/* Fork a child to handle the connection. */
startProcess([&]() {
close(fdSocket);
/* Background the daemon. */
if (setsid() == -1)
throw SysError(format("creating a new session"));
/* Restore normal handling of SIGCHLD. */
setSigChldAction(false);
/* For debugging, stuff the pid into argv[1]. */
if (clientPid != -1 && argvSaved[1]) {
string processName = std::to_string(clientPid);
strncpy(argvSaved[1], processName.c_str(),
strlen(argvSaved[1]));
}
isRemoteConnection = …; /* <– this is the new line */
/* Store the client's user and group for this connection. This
has to be done in the forked process since it is per
connection. Setting these to -1 means: do not change. */
settings.clientUid = clientUid;
settings.clientGid = clientGid;
--8<---------------cut here---------------end--------------->8---
Last thing: could you add a couple of tests? tests/guix-daemon.sh
already has tests for ‘--listen’, so you could take inspiration from
those.
Thank you!
Ludo’.
