From: Roel Janssen <roel@gnu.org>
To: "Ludovic Courtès" <ludovic.courtes@inria.fr>
Cc: guix-devel <guix-devel@gnu.org>
Subject: Re: [PATCH] guix-daemon: Add option to disable garbage collection.
Date: Thu, 19 Apr 2018 17:15:28 +0200 [thread overview]
Message-ID: <87vacnb52y.fsf@gnu.org> (raw)
In-Reply-To: <87tvs7jlsc.fsf@inria.fr>
[-- Attachment #1: Type: text/plain, Size: 2761 bytes --]
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
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-guix-daemon-Disable-garbage-collection-for-remote-co.patch --]
[-- Type: text/x-patch, Size: 2659 bytes --]
From fcbe7ebb3d205cf7310700e62b78b9aafd94f76f Mon Sep 17 00:00:00 2001
From: Roel Janssen <roel@gnu.org>
Date: Thu, 19 Apr 2018 17:11:30 +0200
Subject: [PATCH] guix-daemon: Disable garbage collection for remote
connections.
* 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.
* tests/guix-daemon.sh: Add a test for the new behavior.
---
nix/nix-daemon/nix-daemon.cc | 10 +++++++++-
tests/guix-daemon.sh | 14 ++++++++++++++
2 files changed, 23 insertions(+), 1 deletion(-)
diff --git a/nix/nix-daemon/nix-daemon.cc b/nix/nix-daemon/nix-daemon.cc
index deb7003d7..782e4acfc 100644
--- a/nix/nix-daemon/nix-daemon.cc
+++ b/nix/nix-daemon/nix-daemon.cc
@@ -54,7 +54,9 @@ static FdSink to(STDOUT_FILENO);
bool canSendStderr;
-
+/* This variable is used to keep track of whether a connection
+ comes from a host other than the host running guix-daemon. */
+static bool isRemoteConnection;
/* This function is called anytime we want to write something to
stderr. If we're in a state where the protocol allows it (i.e.,
@@ -529,6 +531,11 @@ static void performOp(bool trusted, unsigned int clientVersion,
}
case wopCollectGarbage: {
+ if (isRemoteConnection) {
+ throw Error("Garbage collection is disabled for remote hosts.");
+ break;
+ }
+
GCOptions options;
options.action = (GCOptions::GCAction) readInt(from);
options.pathsToDelete = readStorePaths<PathSet>(from);
@@ -934,6 +941,7 @@ static void acceptConnection(int fdSocket)
connection. Setting these to -1 means: do not change. */
settings.clientUid = clientUid;
settings.clientGid = clientGid;
+ isRemoteConnection = (remoteAddr.ss_family != AF_UNIX);
/* Handle the connection. */
from.fd = remote;
diff --git a/tests/guix-daemon.sh b/tests/guix-daemon.sh
index 6f91eb58b..9ae6e0b77 100644
--- a/tests/guix-daemon.sh
+++ b/tests/guix-daemon.sh
@@ -194,6 +194,20 @@ do
kill "$daemon_pid"
done
+# Make sure garbage collection from a TCP connection does not work.
+
+tcp_socket="127.0.0.1:9999"
+guix-daemon --listen="$tcp_socket" &
+daemon_pid=$!
+
+GUIX_DAEMON_SOCKET="guix://$tcp_socket"
+export GUIX_DAEMON_SOCKET
+
+if guix gc; then false; else true; fi
+
+unset GUIX_DAEMON_SOCKET
+kill "$daemon_pid"
+
# Log compression.
guix-daemon --listen="$socket" --disable-chroot --debug --log-compression=gzip &
--
2.17.0
next prev parent reply other threads:[~2018-04-19 15:15 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-04-03 10:12 [PATCH] guix-daemon: Add option to disable garbage collection Roel Janssen
2018-04-03 13:40 ` Ludovic Courtès
2018-04-03 14:02 ` Roel Janssen
2018-04-11 7:57 ` Roel Janssen
2018-04-17 21:00 ` Roel Janssen
2018-04-18 21:12 ` Ludovic Courtès
2018-04-19 9:06 ` Ludovic Courtès
2018-04-19 12:12 ` Roel Janssen
2018-04-19 14:47 ` Ludovic Courtès
2018-04-19 15:15 ` Roel Janssen [this message]
2018-04-19 15:26 ` Ludovic Courtès
2018-04-19 17:07 ` Roel Janssen
2018-04-19 15:25 ` Marius Bakke
-- strict thread matches above, loose matches on Subject: below --
2018-04-03 12:41 Adam Van Ymeren
2018-04-03 13:03 ` Roel Janssen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://guix.gnu.org/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87vacnb52y.fsf@gnu.org \
--to=roel@gnu.org \
--cc=guix-devel@gnu.org \
--cc=ludovic.courtes@inria.fr \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/guix.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).