Re: [PATCH] Add statvfs bindings.

[Maxime Devos]

Op 06-07-2023 om 08:17 schreef Marius Bakke:
> * libguile/filesys.c (scm_statvfs2scm): New function.
> (scm_statvfs): New procedure.
> * libguile/filesys.h: Add scm_statvfs.
> * libguile/syscalls.h (fstatvfs_or_fstatvfs64, statvfs_or_statvfs64,
> scm_from_fsblkcnt_t_or_fsblkcnt64_t,
> scm_from_fsfilcnt_t_or_fsfilcnt64_t): New macros.
> * module/ice-9/posix.scm (statvfs:block-size, statvfs:fragment-size,
> statvfs:blocks, statvfs:blocks-free, statvfs:block-available,
> statvfs:files, statvfs:files-free, statvfs:files-available,
> statvfs:fsid, statvfs:flags, statvfs:name-max): New variables.
> * libguile/posix.c (scm_init_posix): Add ST_MANDLOCK, ST_NOATIME,
> ST_NODEV, ST_NODIRATIME, ST_NOEXEC, ST_NOSUID, ST_RDONLY, ST_RELATIME,
> ST_SYNCHRONOUS constants.
> * doc/ref/posix.texi (File System): Document statvfs.
> * test-suite/tests/posix.test ("statvfs"): New tests.
> ---
>   doc/ref/posix.texi          |  50 +++++++++++++++++
>   libguile/filesys.c          | 109 ++++++++++++++++++++++++++++++++++++
>   libguile/filesys.h          |   1 +
>   libguile/posix.c            |  29 ++++++++++
>   libguile/syscalls.h         |   4 ++
>   module/ice-9/posix.scm      |  12 ++++
>   test-suite/tests/posix.test |  39 +++++++++++++
>   7 files changed, 244 insertions(+)
> ---
> Hello Guile,
>
> This patch adds bindings for statvfs(3).  The API is similar to that of `stat`.
> It can be used to implement e.g. `df` in Guile.
>
> I added "long names" for the various getters, instead of following the "POSIX
> identifiers" (block-size instead of bsize, etc).  That may not have been a wise
> decision.  Feedback wanted!

Maybe it's POSIX, but I would use AC_CHECK_FUNCS for this instead of the 
'#ifdef POSIX' anyways -- e.g. in case some new OS wants to mostly 
implement POSIX but isn't all the way there yet.

The long names seem fine to me -- I see no reason to limit ourselves to 
the suboptimal naming choices of POSIX.  And if someone really likes the 
short and less clear names, they could just rename it with a simple 
'define'.

> +static SCM
> +scm_statvfs2scm (struct statvfs_or_statvfs64 *statvfs_temp)
> +{
> +  SCM ans = scm_c_make_vector (11, SCM_UNSPECIFIED);

This is an ad-hoc record type, you could use define-record-type instead 
which exists for exactly this kind of thing.

For this ad-hoc thing, there's precedent for that with 'scm_stat', but 
IMO it's not good precedent, and in case of scm_stat no reason at all 
appears is given for why not use a record type.

As I understand it, the record type would be defined in posix.scm, and 
the C code would access the constructor with (the C equivalent of) a 
'module-ref', or by the method used in libguile/goops.c.

I haven't really looked at the documentation or implementation.

Best regards,
Maxime Devos

OpenPGP_0x49E3EE22191725EE.asc
Description: OpenPGP public key

OpenPGP_signature
Description: OpenPGP digital signature