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

> Roel Janssen <roel@gnu.org> skribis:
>
>> Ludovic Courtès <ludovic.courtes@inria.fr> writes:
>
> [...]
>
>>>> 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?
>
> Sure, that’s what I was suggesting.  :-)
>
>>> 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)?
>
> Run:
>
>   make check TESTS=tests/guix-daemon.sh
>
> See
> <https://www.gnu.org/software/guix/manual/html_node/Running-the-Test-Suite.html>.

That is really nice.  Thanks for pointing to the manual.

>> From b29d3a90e1487ebda5ac5b6bc146f8c95218eab6 Mon Sep 17 00:00:00 2001
>> From: Roel Janssen <roel@gnu.org>
>> Date: Thu, 19 Apr 2018 14:01:49 +0200
>> Subject: [PATCH] guix-daemon: Disable garbage collection for remote hosts.
>>
>> * nix/nix-daemon/nix-daemon.cc (performOp): Display appropriate error message;
>>   (acceptConnection): Set isRemoteConnection when connection is over TCP.
>
> Rather:
>
> * nix/nix-daemon/nix-daemon.cc (isRemoteConnection): New variable.
> (performOp): For wopCollectGarbage, throw an error when
> isRemoteConnection is set.
> (acceptConnection): Set isRemoteConnection when connection is not AF_UNIX.
>
>> +output=`GUIX_DAEMON_SOCKET="$socket" guix gc`
>> +if [[ "$output" != *"GUIX_DAEMON_SOCKET=$socket" ]];
>> +then
>> +    exit 1
>> +fi
>
> Perhaps simply check the exit code of ‘guix gc’ and fail if it succeeds?

Right.

> Like:
>
>   if guix gc; then false; else true; fi
>
> Also please try to avoid Bash-specific constructs like [[ this ]].

Right.

> Could you send an updated patch?

The attached patch should be fine.

Kind regards,
Roel Janssen