From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ioannis Panagiotis Koutsidis Subject: Re: GSoC 2018 Syntax and semantics of systemd units in the Shepherd - 1st update Date: Mon, 11 Jun 2018 15:07:23 +0300 Message-ID: <9b3b976b-3a3f-d5b6-2aff-8200314f7b7a@student.bham.ac.uk> References: <87zi01ilyh.fsf@gnu.org> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:46208) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fSLbk-0007Aa-4Z for guix-devel@gnu.org; Mon, 11 Jun 2018 08:07:39 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fSLbg-0002EI-H5 for guix-devel@gnu.org; Mon, 11 Jun 2018 08:07:36 -0400 In-Reply-To: <87zi01ilyh.fsf@gnu.org> Content-Language: en-GB 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: =?UTF-8?Q?Ludovic_Court=c3=a8s?= Cc: guix-devel@gnu.org Thank you a lot for your comments! I will make sure to make the changes that you suggested. As for match and things like car/cdr, I had issues with match and signal handling in the service file, which was why I changed it with a cond. As for the unit parser I also take the rest of the list via cdar because match in something like (x y rest ...) does not bind rest - I will probably have to use (x . (y . rest)) in the replacement. On 06/11/18 14:47, Ludovic Courtès wrote: > 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 supported >> 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-abort) >> 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! > >> During the next phases I will focus on other common .service entries, .socket >> support, as well as thoroughly testing the code. > > Cool. Adding unit tests like those currently under tests/ is definitely > something you should do—you probably already run tests manually anyway, > so it’s 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 ‘SCM_LOG_COMPILER’ 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 “logical” change? > > By that I mean that you could have a first patch that modifies ‘restart’ > 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 ‘git log’ and basically try to mimic what’s been done > before. Don’t lose your hair over commit logs though; it’s good to try > to follow the conventions, but if you’re unsure or if you make mistakes, > it’s not the end of the world. > >> @@ -165,6 +166,11 @@ respawned, shows that it has been respawned more than 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 ‘respawn?’ must simply be renamed to ‘respawn’ (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 ‘=’ rather than ‘equal?’ when we know we’re 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 ‘eq?’ 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 modify 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, but >> +;; 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’ll have to call ‘read-char’ instead of > traversing the list of characters, but otherwise the code should be > pretty much the same. > > How does that sound? > > Also, I’d use the name ‘read-unit-file’ for this procedure, 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 missing 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)] >> + [(#\= 'keypart) >> + (unit-parse rest >> + 'valuepart >> + key >> + '() >> + kv)] >> + [(#\newline 'valuepart) >> + (unit-parse rest >> + 'firstchar >> + '() >> + '() >> + `((,(list->string key) >> + . ,(list->string value)) >> + . ,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 ‘letrec’, which leads to code that goes far to the right, you > can use ‘define’ like this (it’s equivalent): > > (define (read-unit-file port) > (define (parse s state key value kv) > …) > (parse …)) > > Please always use ‘match’ instead of ‘car’, ‘cdr’, 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 ‘read’ procedure and no ‘read-file’ 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 currently used >> + [workdir (dassoc "WorkingDirectory" alst rootdir)] >> + [command execstart]) > > The (dassoc …) 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 “good enough”, so once you have a unit test for > it, I’d suggest moving to implementing the semantics of unit files > (which you’ve 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’. >