GNU bug report logs - #27977
[PATCH] services: herd: Fix matching ok responses and add stop service procedure

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 27977 in the body.
You can then email your comments to 27977 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: guix-patches <at> gnu.org
Subject: [PATCH] services: herd: Fix matching ok responses and add stop
 service procedure
Date: Sat, 5 Aug 2017 22:26:46 +0100

[Message part 1 (text/plain, inline)]

Patches incoming...

[Message part 2 (application/pgp-signature, inline)]

Message #8 received at 27977 <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: 27977 <at> debbugs.gnu.org
Subject: [PATCH 2/2] services: herd: Add a stop-service procedure.
Date: Sat,  5 Aug 2017 22:30:34 +0100

* gnu/services/herd.scm (stop-service): New procedure.
---
 gnu/services/herd.scm | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/gnu/services/herd.scm b/gnu/services/herd.scm
index 49400aba4..e16d51b9d 100644
--- a/gnu/services/herd.scm
+++ b/gnu/services/herd.scm
@@ -49,7 +49,8 @@
             unload-services
             unload-service
             load-services
-            start-service))
+            start-service
+            stop-service))
 
 ;;; Commentary:
 ;;;
@@ -222,6 +223,10 @@ returns a shepherd <service> object."
   (with-shepherd-action name ('start) result
     result))
 
+(define (stop-service name)
+  (with-shepherd-action name ('stop) result
+    result))
+
 ;; Local Variables:
 ;; eval: (put 'alist-let* 'scheme-indent-function 2)
 ;; eval: (put 'with-shepherd 'scheme-indent-function 1)
-- 
2.13.1

Message #11 received at 27977 <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: 27977 <at> debbugs.gnu.org
Subject: [PATCH 1/2] services: herd: Fix matching ok responses from shepherd
 service.
Date: Sat,  5 Aug 2017 22:30:33 +0100

Previously the match expression case for a successful response
(where error is #f) required that the result component contained a list with a
single element.

As far as I see when looking at the responses from the shepherd, this is not
normally the case. Therefore, to avoid treating successful responses as
errors, make the match requirement more permissive, accepting any value.

* gnu/services/herd.scm (invoke-action): Change match condition for ok responses.
---
 gnu/services/herd.scm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gnu/services/herd.scm b/gnu/services/herd.scm
index f8d60a480..49400aba4 100644
--- a/gnu/services/herd.scm
+++ b/gnu/services/herd.scm
@@ -146,7 +146,7 @@ result.  Otherwise return #f."
     (force-output sock)
 
     (match (read sock)
-      (('reply ('version 0 _ ...) ('result (result)) ('error #f)
+      (('reply ('version 0 _ ...) ('result result) ('error #f)
                ('messages messages))
        (for-each display-message messages)
        (cont result))
-- 
2.13.1

Message #14 received at 27977 <at> debbugs.gnu.org (full text, mbox):

From: Danny Milosavljevic <dannym <at> scratchpost.org>
To: Christopher Baines <mail <at> cbaines.net>
Cc: 27977 <at> debbugs.gnu.org
Subject: Re: [bug#27977] [PATCH 2/2] services: herd: Add a stop-service
 procedure.
Date: Tue, 8 Aug 2017 10:14:01 +0200

LGTM!

Message #17 received at 27977 <at> debbugs.gnu.org (full text, mbox):

From: Danny Milosavljevic <dannym <at> scratchpost.org>
To: Christopher Baines <mail <at> cbaines.net>
Cc: 27977 <at> debbugs.gnu.org
Subject: Re: [bug#27977] [PATCH 1/2] services: herd: Fix matching ok
 responses from shepherd service.
Date: Tue, 8 Aug 2017 10:16:42 +0200

LGTM!

Message #22 received at 27977-done <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: Danny Milosavljevic <dannym <at> scratchpost.org>
Cc: 27977-done <at> debbugs.gnu.org
Subject: Re: [bug#27977] [PATCH 2/2] services: herd: Add a stop-service
 procedure.
Date: Tue, 8 Aug 2017 20:53:04 +0100

[Message part 1 (text/plain, inline)]

On Tue, 8 Aug 2017 10:14:01 +0200
Danny Milosavljevic <dannym <at> scratchpost.org> wrote:

> LGTM!

Thanks for your review Danny.

I've now pushed :)

[Message part 2 (application/pgp-signature, inline)]

Message #25 received at 27977 <at> debbugs.gnu.org (full text, mbox):

From: ludo <at> gnu.org (Ludovic Courtès)
To: Christopher Baines <mail <at> cbaines.net>
Cc: 27977 <at> debbugs.gnu.org
Subject: Re: [bug#27977] [PATCH 1/2] services: herd: Fix matching ok responses
 from shepherd service.
Date: Tue, 22 Aug 2017 14:39:36 +0200

Christopher Baines <mail <at> cbaines.net> skribis:

> Previously the match expression case for a successful response
> (where error is #f) required that the result component contained a list with a
> single element.

Good catch!

Ludo’.

Message #28 received at 27977 <at> debbugs.gnu.org (full text, mbox):

From: ludo <at> gnu.org (Ludovic Courtès)
To: Christopher Baines <mail <at> cbaines.net>
Cc: 27977 <at> debbugs.gnu.org
Subject: Re: [bug#27977] [PATCH 1/2] services: herd: Fix matching ok responses
 from shepherd service.
Date: Tue, 22 Aug 2017 17:52:44 +0200

[Message part 1 (text/plain, inline)]

Christopher Baines <mail <at> cbaines.net> skribis:

> Previously the match expression case for a successful response
> (where error is #f) required that the result component contained a list with a
> single element.
>
> As far as I see when looking at the responses from the shepherd, this is not
> normally the case. Therefore, to avoid treating successful responses as
> errors, make the match requirement more permissive, accepting any value.
>
> * gnu/services/herd.scm (invoke-action): Change match condition for ok responses.
> ---
>  gnu/services/herd.scm | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/gnu/services/herd.scm b/gnu/services/herd.scm
> index f8d60a480..49400aba4 100644
> --- a/gnu/services/herd.scm
> +++ b/gnu/services/herd.scm
> @@ -146,7 +146,7 @@ result.  Otherwise return #f."
>      (force-output sock)
>  
>      (match (read sock)
> -      (('reply ('version 0 _ ...) ('result (result)) ('error #f)
> +      (('reply ('version 0 _ ...) ('result result) ('error #f)
>                 ('messages messages))

Actually this is not OK (it broke system tests because
‘current-services’ was now getting a single-element list instead of the
list of service sexps.)

The reason for this is that the ‘action’ method in the Shepherd, when
invoked on a symbol, returns a list of results, one for each service of
that name:

  (define-method (action (obj <symbol>) the-action . args)
    "Perform THE-ACTION on all the services named OBJ.  Return the list of
  results."
    (let ((which-services (lookup-running-or-providing obj)))
      (if (null? which-services)
          (let ((unknown (lookup-running 'unknown)))
            (if (and unknown
                     (defines-action? unknown 'action))
                (apply action unknown 'action the-action args)
                (raise (condition (&missing-service-error (name obj))))))
          (map (lambda (service)
                 (apply action service the-action args))
               which-services))))

(With the exception of actions called on the ‘unknown’ service, which we
should probably get rid of.)

So either we revert the change, or we do this:

[Message part 2 (text/x-patch, inline)]

diff --git a/gnu/services/herd.scm b/gnu/services/herd.scm
index e16d51b9d..7614c7f9f 100644
--- a/gnu/services/herd.scm
+++ b/gnu/services/herd.scm
@@ -1,5 +1,5 @@
 ;;; GNU Guix --- Functional package management for GNU
-;;; Copyright © 2016 Ludovic Courtès <ludo <at> gnu.org>
+;;; Copyright © 2016, 2017 Ludovic Courtès <ludo <at> gnu.org>
 ;;; Copyright © 2017 Mathieu Othacehe <m.othacehe <at> gmail.com>
 ;;;
 ;;; This file is part of GNU Guix.
@@ -186,7 +186,11 @@ of pairs."
   "Return the list of currently defined Shepherd services, represented as
 <live-service> objects.  Return #f if the list of services could not be
 obtained."
-  (with-shepherd-action 'root ('status) services
+  (with-shepherd-action 'root ('status) results
+    ;; We get a list of results, one for each service with the name 'root'.
+    ;; In practice there's only one such service though.
+    (match results
+      ((services _ ...)
        (match services
          ((('service ('version 0 _ ...) _ ...) ...)
           (map (lambda (service)
@@ -194,22 +198,22 @@ obtained."
                    (live-service provides requires running)))
                services))
          (x
-       #f))))
+          #f))))))
 
 (define (unload-service service)
   "Unload SERVICE, a symbol name; return #t on success."
   (with-shepherd-action 'root ('unload (symbol->string service)) result
-    result))
+    (first result)))
 
 (define (%load-file file)
   "Load FILE in the Shepherd."
   (with-shepherd-action 'root ('load file) result
-    result))
+    (first result)))
 
 (define (eval-there exp)
   "Eval EXP in the Shepherd."
   (with-shepherd-action 'root ('eval (object->string exp)) result
-    result))
+    (first result)))
 
 (define (load-services files)
   "Load and register the services from FILES, where FILES contain code that

[Message part 3 (text/plain, inline)]

Probably this patch is better than reverting.

Thoughts?

Ludo’.

Message #31 received at 27977 <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: ludo <at> gnu.org (Ludovic Courtès)
Cc: 27977 <at> debbugs.gnu.org
Subject: Re: [bug#27977] [PATCH 1/2] services: herd: Fix matching ok
 responses from shepherd service.
Date: Tue, 22 Aug 2017 17:44:52 +0100

[Message part 1 (text/plain, inline)]

On Tue, 22 Aug 2017 17:52:44 +0200
ludo <at> gnu.org (Ludovic Courtès) wrote:

> Probably this patch is better than reverting.
> 
> Thoughts?

I had to apply that patch with --ignore-whitespace-change, as the code
in the middle of (current-services) has been indented outside of that
patch.

I think I get what is going on. As far as I understand it, the (match
results ((services _ ...) ... bit is equivilent to the use of first in
the other procedures, which suggests to me that you could use first in
(current-services)? I'm guessing that the only difference is that they
will fail differently on the empty list?

Also, I've successfully ran the memcached service test with this
change, so there is no regression there which is good :)

[Message part 2 (application/pgp-signature, inline)]

Message #34 received at 27977-done <at> debbugs.gnu.org (full text, mbox):

From: ludo <at> gnu.org (Ludovic Courtès)
To: Christopher Baines <mail <at> cbaines.net>
Cc: 27977-done <at> debbugs.gnu.org
Subject: Re: [bug#27977] [PATCH 1/2] services: herd: Fix matching ok responses
 from shepherd service.
Date: Wed, 23 Aug 2017 00:30:21 +0200

Christopher Baines <mail <at> cbaines.net> skribis:

> On Tue, 22 Aug 2017 17:52:44 +0200
> ludo <at> gnu.org (Ludovic Courtès) wrote:
>
>> Probably this patch is better than reverting.
>> 
>> Thoughts?
>
> I had to apply that patch with --ignore-whitespace-change, as the code
> in the middle of (current-services) has been indented outside of that
> patch.

Right, sorry about that (I have -bB in ‘vc-diff-switches’).

> I think I get what is going on. As far as I understand it, the (match
> results ((services _ ...) ... bit is equivilent to the use of first in
> the other procedures, which suggests to me that you could use first in
> (current-services)? I'm guessing that the only difference is that they
> will fail differently on the empty list?

Yes.  I’ve pushed it as 7d14082d56462f7bef4254d65a21fd265fbce471.

Thanks,
Ludo’.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.