GNU bug report logs - #34863
[WIP] syscalls: Add loop device interface.

Previous Next

Full log

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

From: Ludovic Courtès <ludo <at> gnu.org>
To: Danny Milosavljevic <dannym <at> scratchpost.org>
Cc: 34863 <at> debbugs.gnu.org
Subject: Re: [bug#34863] [WIP] syscalls: Add loop device interface.
Date: Sat, 16 Mar 2019 11:29:17 +0100

Hallo!

Danny Milosavljevic <dannym <at> scratchpost.org> 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 ‘lo-’ 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’t know about ‘open-io-file’ and indeed, it’s undocumented.  So
I’d suggest using ‘open-file’ 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: “Set BACKING-FILE, a port, as the backing
file of LOOP-FILE, an open port to a loopback device.”?

> +  (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’s GC’d, 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’d have to use ‘port->fdes’ instead of ‘fileno’ to
increase the “revealed count” of the port.

> +(define (get-loop-device-status loop-file)

Please add a docstring.  Also I’d remove ‘get-’.

> +(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))))
>  
> +(let ((loop-device (allocate-new-loop-device (open-io-file "/dev/loop-control"))))
> +  (set-loop-device-backing-file loop-device (open-input-file "tests/syscalls.scm"))
> +  (set-loop-device-status loop-device (get-loop-device-status loop-device)))

You’re missing a ‘test-assert’ or similar.  Also, isn’t ‘loop-device’ a
number?  Then the ‘set-loop-device-*’ calls fail with wrong-type-arg,
no?

Thank you!

Ludo’.

Previous Next

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