From mboxrd@z Thu Jan 1 00:00:00 1970 From: ludo@gnu.org (Ludovic =?utf-8?Q?Court=C3=A8s?=) Subject: Re: GSoC 2018 Syntax and semantics of systemd units in the Shepherd - 1st update Date: Mon, 11 Jun 2018 13:47:34 +0200 Message-ID: <87zi01ilyh.fsf@gnu.org> References: Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:39138) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fSLIT-0007Bs-2S for guix-devel@gnu.org; Mon, 11 Jun 2018 07:47:44 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fSLIP-0004zM-QW for guix-devel@gnu.org; Mon, 11 Jun 2018 07:47:41 -0400 In-Reply-To: (Ioannis Panagiotis Koutsidis's message of "Mon, 11 Jun 2018 06:02:46 +0300") 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: Ioannis Panagiotis Koutsidis Cc: guix-devel@gnu.org Hello Ioannis! Thanks for the update! Ioannis Panagiotis Koutsidis skribis: > As the 1st phase is coming to an end I decided to post my progress. I have > implemented the unit file parsing as well as some of the basic entries su= pported > by it, such as ExecStart, User, Group, Restart, etc. In addition, support= for > the systemd Restart values (on-success, on-failure, on-abnormal, and on-a= bort) > was added to the Shepherd via the restart-systemd field in the = class, > letting services written in guile to also use that feature. Very nice!=20=20 > During the next phases I will focus on other common .service entries, .so= cket > support, as well as thoroughly testing the code. Cool. Adding unit tests like those currently under tests/ is definitely something you should do=E2=80=94you probably already run tests manually any= way, so it=E2=80=99s mostly a matter of putting them in a file. For things like the unit file parser, you may find it more convenient to write the test in Scheme (currently all the tests are shell scripts.) That can easily be done by using the .scm file name extension for your test and then defining =E2=80=98SCM_LOG_COMPILER=E2=80=99 in Makefile.am. = If unsure, you can look at how Guix itself does it, or just stop by on #guix or ask on the list for details. Some comments about the code: > From a0a46ead5e43cd2672a08adb4c16919c377514c2 Mon Sep 17 00:00:00 2001 > From: Ioannis Panagiotis Koutsidis > Date: Sat, 9 Jun 2018 16:17:27 +0300 > Subject: [PATCH] Initial systemd unit support Could you try to split it in several patches, where each patch represents a single =E2=80=9Clogical=E2=80=9D change? By that I mean that you could have a first patch that modifies =E2=80=98res= tart=E2=80=99 and all in (shepherd service), possibly with extended tests to exercise the new functionality if appropriate. A second patch would add the unit file parser in (shepherd systemd) along with its unit test. For commit logs, please try to follow the ChangeLog convention: . You can look at =E2=80=98git log=E2=80=99 and basically try to mimic what=E2=80= =99s been done before. Don=E2=80=99t lose your hair over commit logs though; it=E2=80=99s= good to try to follow the conventions, but if you=E2=80=99re unsure or if you make mist= akes, it=E2=80=99s not the end of the world. > @@ -165,6 +166,11 @@ respawned, shows that it has been respawned more tha= n TIMES in SECONDS." > (respawn? #:init-keyword #:respawn? > #:init-value #f > #:getter respawn?) > + ;; For the systemd restart values. Can be 'no (when respawn? is #f), > + ;; 'on-success, 'on-failure, 'on-abnormal, 'on-watchdog, 'on-abort, or= 'always > + (respawn-systemd #:init-keyword #:respawn-systemd > + #:init-value 'always > + #:getter respawn-systemd) As briefly discussed on IRC, I think we should keep a single field for this. So perhaps =E2=80=98respawn?=E2=80=99 must simply be renamed to =E2= =80=98respawn=E2=80=99 (no question mark), with a comment like above explaining what the possible values are. > + (let* ([e (status:exit-val status)] > + [t (status:term-sig status)] > + [r (respawn-systemd serv)] Please avoid square brackets to remain consistent with the rest of the code. :-) > + [clean (or (zero? e) > + (equal? t SIGHUP) > + (equal? t SIGINT) > + (equal? t SIGTERM) > + (equal? t SIGPIPE))]) Use =E2=80=98=3D=E2=80=99 rather than =E2=80=98equal?=E2=80=99 when we know= we=E2=80=99re dealing with numbers. > + (if (or (equal? r 'always) > + (equal? r 'on-watchdog) ;; not implemented yet > + (and (equal? r 'on-success) clean) > + (and (equal? r 'on-abnormal) (not clean) (equal? e #f)) > + (and (equal? r 'on-failure) (not clean)) > + (and (equal? r 'on-abort) (equal? t SIGABRT))) Likewise, use =E2=80=98eq?=E2=80=99 for symbols. > +++ b/modules/shepherd/systemd.scm > @@ -0,0 +1,143 @@ > +;; systemd.scm -- Systemd support > +;; Copyright (C) 2018 Ioannis Panagiotis Koutsidis > +;; > +;; This file is part of the GNU Shepherd. > +;; > +;; The GNU Shepherd is free software; you can redistribute it and/or mod= ify it > +;; under the terms of the GNU General Public License as published by > +;; the Free Software Foundation; either version 3 of the License, or (at > +;; your option) any later version. > +;; > +;; The GNU Shepherd is distributed in the hope that it will be useful, b= ut > +;; WITHOUT ANY WARRANTY; without even the implied warranty of > +;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > +;; GNU General Public License for more details. > +;; > +;; You should have received a copy of the GNU General Public License > +;; along with the GNU Shepherd. If not, see . > + > +(define-module (shepherd systemd) > + #:use-module (ice-9 match) > + #:use-module (ice-9 textual-ports) > + #:use-module (oop goops) > + #:use-module (shepherd service) > + #:export (make-systemd-service)) > + > +;; Change this > +(define unitdir "/systemd/") I think we can remove it altogether. :-) > +;; Implements a state machine to parse the ini-like systemd unit files > +(define (unit-parse s) Please turn the comment into a docstring. Also, it may be more idiomatic to take an input port instead of a string as input. As a result, you=E2=80=99ll have to call =E2=80=98read-char=E2= =80=99 instead of traversing the list of characters, but otherwise the code should be pretty much the same. How does that sound? Also, I=E2=80=99d use the name =E2=80=98read-unit-file=E2=80=99 for this pr= ocedure, which is more inline with some naming conventions. > + (letrec ([unit-parse (lambda (s state key value kv) > + (match (list s state) > + [((or (#\newline _ ...) > + ()) 'keypart) > + (error "Key " (list->string key) " is missin= g its value")] > + [(() (or 'valuepart 'firstchar 'ignoreline)) > + kv] > + [lst (let ([rest (cdar lst)]) > + (match (list (caar lst) state) > + [((or #\; > + #\[) 'firstchar) > + (unit-parse rest > + 'ignoreline > + '() > + '() > + kv)] > + [(#\newline (or 'firstchar > + 'ignoreline)) > + (unit-parse rest > + 'firstchar > + '() > + '() > + kv)] > + [(#\=3D 'keypart) > + (unit-parse rest > + 'valuepart > + key > + '() > + kv)] > + [(#\newline 'valuepart) > + (unit-parse rest > + 'firstchar > + '() > + '() > + `((,(list->string key) > + . ,(list->string val= ue)) > + . ,kv))] > + [(_ 'ignoreline) > + (unit-parse rest > + 'ignoreline > + '() > + '() > + kv)] > + [(c 'valuepart) > + (unit-parse rest > + 'valuepart > + key > + (append value `(,c)) > + kv)] > + [(c (or 'keypart 'firstchar)) > + (unit-parse rest > + 'keypart > + (append key `(,c)) > + '() > + kv)]))]))]) Instead of =E2=80=98letrec=E2=80=99, which leads to code that goes far to t= he right, you can use =E2=80=98define=E2=80=99 like this (it=E2=80=99s equivalent): (define (read-unit-file port) (define (parse s state key value kv) =E2=80=A6) (parse =E2=80=A6)) Please always use =E2=80=98match=E2=80=99 instead of =E2=80=98car=E2=80=99,= =E2=80=98cdr=E2=80=99, etc., and avoid abbreviations. See , which mostly applies to the Shepherd as well. > +(define (unit-parse-file path) > + (let* ([in (open-input-file path)] > + [out (unit-parse (get-string-all in))]) > + (close-port in) > + out)) This will probably not be needed anymore (just like Scheme itself provides a =E2=80=98read=E2=80=99 procedure and no =E2=80=98read-file=E2=80= =99 procedure.) > +;; like assoc but uses a coninuation for failure and success > +(define (kassoc key alst failure success) > + (let ((res (assoc key alst))) > + (if (equal? res #f) > + failure > + (success (cdr res))))) > + > +;; like assoc but 1: allows the use of a default value on failure > +;; and 2: returns just the value instead of (cons key value) > +(define (dassoc key alst default) > + (kassoc key alst default (lambda (x) x))) Most likely these are not needed. :-) > +(define (make-systemd-service name) Rather: (unit-file->service port) ? > + (let* ([alst (unit-parse-file (string-append unitdir name))] > + [busname (dassoc "BusName" alst #f)] > + [execstart (dassoc "ExecStart" alst #f)] > + [type (dassoc "Type" alst (if (equal? execstart #f) > + "oneshot" > + (if (equal? busname #f) > + "simple" > + "dbus")))] > + [restart (string->symbol (dassoc "Restart" alst "no"))] > + [user (dassoc "User" alst #f)] > + [group (dassoc "Group" alst #f)] > + [rootdir (dassoc "RootDirectory" alst "/")] ;; not c= urrently used > + [workdir (dassoc "WorkingDirectory" alst rootdir)] > + [command execstart]) The (dassoc =E2=80=A6) above can be replaced by: (assoc-ref alist "Thing") or: (or (assoc-ref alist "Thing") 'default-thing) > +(register-services (make-systemd-service "test.service")) This should go to the unit test. The unit parser looks =E2=80=9Cgood enough=E2=80=9D, so once you have a uni= t test for it, I=E2=80=99d suggest moving to implementing the semantics of unit files (which you=E2=80=99ve started a bit.) You may find that some things, such = as socket activation, are hard to implement in the current code. At that point we can start discussing how to do that, which will probably mean moving using Fibers to handle events. Thank you, Ludo=E2=80=99.