From debbugs-submit-bounces@debbugs.gnu.org Sat Mar 16 06:29:28 2019 Received: (at 34863) by debbugs.gnu.org; 16 Mar 2019 10:29:28 +0000 Received: from localhost ([127.0.0.1]:45801 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1h56ZE-0008C1-1G for submit@debbugs.gnu.org; Sat, 16 Mar 2019 06:29:28 -0400 Received: from eggs.gnu.org ([209.51.188.92]:37201) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1h56ZC-0008Bk-2D for 34863@debbugs.gnu.org; Sat, 16 Mar 2019 06:29:26 -0400 Received: from fencepost.gnu.org ([2001:470:142:3::e]:57750) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1h56Z6-0000eL-Pc; Sat, 16 Mar 2019 06:29:20 -0400 Received: from [2a01:e0a:1d:7270:af76:b9b:ca24:c465] (port=33970 helo=ribbon) by fencepost.gnu.org with esmtpsa (TLS1.2:RSA_AES_256_CBC_SHA1:256) (Exim 4.82) (envelope-from ) id 1h56Z5-00052g-DL; Sat, 16 Mar 2019 06:29:20 -0400 From: =?utf-8?Q?Ludovic_Court=C3=A8s?= To: Danny Milosavljevic Subject: Re: [bug#34863] [WIP] syscalls: Add loop device interface. References: <20190314220823.30769-1-dannym@scratchpost.org> Date: Sat, 16 Mar 2019 11:29:17 +0100 In-Reply-To: <20190314220823.30769-1-dannym@scratchpost.org> (Danny Milosavljevic's message of "Thu, 14 Mar 2019 23:08:23 +0100") Message-ID: <87k1gzyuwy.fsf@gnu.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Spam-Score: 0.0 (/) X-Debbugs-Envelope-To: 34863 Cc: 34863@debbugs.gnu.org X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: debbugs-submit-bounces@debbugs.gnu.org Sender: "Debbugs-submit" X-Spam-Score: -1.0 (-) Hallo! Danny Milosavljevic skribis: > * guix/build/syscalls.scm (%ioctl-unsigned-long): New procedure. > (LOOP_CTL_GET_FREE): New macro. > (LOOP_SET_FD): New macro. > (LOOP_SET_STATUS64): New macro. > (LOOP_GET_STATUS64): New macro. > (lo-flags): New bits. > (lo-flags->symbols): New procedure. > (LO_NAME_SIZE): New variable. > (LO_KEY_SIZE): New variable. > (%struct-loop-info64): New C structure. > (allocate-new-loop-device): New procedure. > (set-loop-device-backing-file): New procedure. > (get-loop-device-status): New procedure. > * tests/syscalls.scm: Add test. What will be the use for this? I prefer to make sure we only add code that is actually going to be used. :-) > +(define-c-struct %struct-loop-info64 > + sizeof-loop-info64 > + (lambda (lo-device lo-inode lo-rdevice lo-offset lo-sizelimit lo-number > + lo-encrypt-type lo-encrypt-key-size lo-flags lo-file-name > + lo-crypt-name lo-encrypt-key lo-init) > + `((lo-device . ,lo-device) > + (lo-inode . ,lo-inode) Like I wrote, a record may be more appropriate than an alist here. Also, no need to repeat =E2=80=98lo-=E2=80=99 in the parameter names. > +(define (allocate-new-loop-device control-file) > + "Allocates a new loop device and returns an FD for it. > +CONTROL-FILE should be an open file \"/dev/loop-control\". Nitpick: s/an FD/a file descriptor/ s/an open file/an open port for/ > + (open-io-file (string-append "/dev/loop" (number->string ret)))) I didn=E2=80=99t know about =E2=80=98open-io-file=E2=80=99 and indeed, it= =E2=80=99s undocumented. So I=E2=80=99d suggest using =E2=80=98open-file=E2=80=99 instead to be on the = safe side. > +(define (set-loop-device-backing-file loop-file backing-file) > + "Sets up the loop device LOOP-FILE for BACKING-FILE." Maybe the docstring should be: =E2=80=9CSet BACKING-FILE, a port, as the ba= cking file of LOOP-FILE, an open port to a loopback device.=E2=80=9D? > + (let-values (((ret err) > + (%ioctl-unsigned-long (fileno loop-file) LOOP_SET_FD > + (fileno backing-file)))) Note that BACKING-FILE, the port, can be closed when it=E2=80=99s GC=E2=80= =99d, which as a side effect would close its associated file descriptor. Is this OK or does the FD have to remain open for the lifetime of the loopback device? In the latter case you=E2=80=99d have to use =E2=80=98port->fdes=E2=80=99 i= nstead of =E2=80=98fileno=E2=80=99 to increase the =E2=80=9Crevealed count=E2=80=9D of the port. > +(define (get-loop-device-status loop-file) Please add a docstring. Also I=E2=80=99d remove =E2=80=98get-=E2=80=99. > +(define (set-loop-device-status loop-file status) Docstring! :-) > --- a/tests/syscalls.scm > +++ b/tests/syscalls.scm > @@ -564,6 +564,10 @@ > (let ((result (call-with-input-file "/var/run/utmpx" read-utmpx))) > (or (utmpx? result) (eof-object? result)))) >=20=20 > +(let ((loop-device (allocate-new-loop-device (open-io-file "/dev/loop-co= ntrol")))) > + (set-loop-device-backing-file loop-device (open-input-file "tests/sysc= alls.scm")) > + (set-loop-device-status loop-device (get-loop-device-status loop-devic= e))) You=E2=80=99re missing a =E2=80=98test-assert=E2=80=99 or similar. Also, i= sn=E2=80=99t =E2=80=98loop-device=E2=80=99 a number? Then the =E2=80=98set-loop-device-*=E2=80=99 calls fail with wrong= -type-arg, no? Thank you! Ludo=E2=80=99.