From mboxrd@z Thu Jan 1 00:00:00 1970 From: Roel Janssen Subject: Re: [PATCH] guix-daemon: Add option to disable garbage collection. Date: Thu, 19 Apr 2018 14:12:02 +0200 Message-ID: <871sfbcs5t.fsf@gnu.org> References: <87a7uklht1.fsf@gnu.org> <87po3g9znv.fsf@gnu.org> <87vad8o0av.fsf@gnu.org> <874lkijhts.fsf@gnu.org> <87sh7rmuot.fsf@inria.fr> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:38170) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1f98QI-0003nS-LN for guix-devel@gnu.org; Thu, 19 Apr 2018 08:12:24 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1f98QC-0006NZ-EU for guix-devel@gnu.org; Thu, 19 Apr 2018 08:12:22 -0400 In-reply-to: <87sh7rmuot.fsf@inria.fr> List-Id: "Development of GNU Guix and the GNU System distribution." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org Sender: "Guix-devel" To: Ludovic =?utf-8?Q?Court=C3=A8s?= Cc: guix-devel --=-=-= Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Ludovic Courtès writes: > Hello Roel, > > Roel Janssen skribis: > [...] > >> From 00f489d6303720c65571fdf0bc9ee810a20f70e0 Mon Sep 17 00:00:00 2001 >> From: Roel Janssen >> 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(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 --=-=-= Content-Type: text/x-patch Content-Disposition: attachment; filename=0001-guix-daemon-Disable-garbage-collection-for-remote-ho.patch >From b29d3a90e1487ebda5ac5b6bc146f8c95218eab6 Mon Sep 17 00:00:00 2001 From: Roel Janssen 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. * 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(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..438c79c26 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. + +socket="127.0.0.1:9999" +guix-daemon --listen="$socket" & +daemon_pid=$! + +output=`GUIX_DAEMON_SOCKET="$socket" guix gc` +if [[ "$output" != *"GUIX_DAEMON_SOCKET=$socket" ]]; +then + exit 1 +fi + +kill "$daemon_pid" + # Log compression. guix-daemon --listen="$socket" --disable-chroot --debug --log-compression=gzip & -- 2.17.0 --=-=-=--