Ludovic Courtès <ludovic.courtes@inria.fr> writes:

> Hello Roel,
>
> Roel Janssen <roel@gnu.org> skribis:
>

[...]

>
>> From 00f489d6303720c65571fdf0bc9ee810a20f70e0 Mon Sep 17 00:00:00 2001
>> From: Roel Janssen <roel@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.)

Right.  It is indeed unnecessary.  In the updated patch, I no longer do this.

>
>> 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.

Could we please start with a “disable any GC” and start allowing cases
on a case-by-case basis?  The reason I request this, is because it makes
it a lot easier to reason about it from a sysadmin point-of-view.

I'd like to think of it like this: “Garbage collection is effectively
turned off.”.  With the current patch, we can reason about it this way.

>
>> @@ -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---

Right.  I implemented it using a static global variable in
nix-daemon.cc.  This patch gets shorter and shorter. :)

>
> 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.

I included a test, but I don't know how I can properly run this test.
Could you elaborate on how I can test the test(s)?

Kind regards,
Roel Janssen