Re: [PATCH] getifaddrs wrapper

[Ludovic Courtès]

Rohan Prinja <address@hidden> skribis:

> From 668910afbe979145a7699708817e28d219ec0750 Mon Sep 17 00:00:00 2001
> From: Rohan Prinja <address@hidden>
> Date: Fri, 19 Jun 2015 12:05:05 +0530
> Subject: [PATCH] add wrapper for getifaddrs (3) to guix/build/syscalls.scm

Nice!

I have a few comments, but nothing major.

Please add a copyright line for yourself in the file.

Please do not use tabs at all in the file.

Could you add a test in the tests/syscalls.scm file?  Basically
something that makes sure that ‘getifaddrs’ returns a (possibly empty)
list, with relevant values.

> +;; TODO: no support for unions yet.

It’s not clear what’s “to be done” here.  Could you rephrase it maybe?

> +;; This only supports broadcast addrs.

Ditto.

> +(define-c-struct ifaddrs                          ;<ifaddrs.h>
> +  read-ifaddrs
> +  write-ifaddrs!
> +  (ifa-next '*)
> +  (ifa-name '*)
> +  (ifa-flags unsigned-int)
> +  (ifa-addr '*)
> +  (ifa-netmask '*)
> +  (ifu-broadcastaddr '*)
> +  (ifa-data '*))

Currently ‘read-ifaddrs’ and ‘write-ifaddrs!’ are unused, but they
should be used (see below.)

> +(define-syntax-rule (bytevector-slice bv start len)

Please change ‘define-syntax-rule’ to ‘define’ and add a docstring.

> +  (let* ((res (make-bytevector len 0))
> +      (_ (bytevector-copy! bv start res 0 len)))
> +    res))

Rather:

  (let ((result (make-bytevector len)))
    (bytevector-copy! bv start result 0 len)
    result)

> +;; See getifaddrs (3) for a description of
> +;; struct ifaddrs.
> +(define %struct-ifaddrs-type
> +  `(* * ,unsigned-int * * * *))

The comment should be “FFI type for ‘struct ifaddr’.”

> +(define %getifaddrs
> +  (let* ((ptr (dynamic-func "getifaddrs" (dynamic-link)))
> +       (proc (pointer->procedure int ptr (list '*)))
> +       (struct-init (list %null-pointer
> +                          %null-pointer
> +                          0
> +                          %null-pointer
> +                          %null-pointer
> +                          %null-pointer
> +                          %null-pointer)))
> +    (lambda ()
> +      "Wrapper around getifaddrs (3)."
> +      (let* ((ifap (make-c-struct %struct-ifaddrs-type
> +                               struct-init))
> +          (ifapp (scm->pointer ifap)) ; ifap ptr

s/ifapp/ptr/ and s/scm->pointer/pointer-address/ because ‘make-c-struct’
returns a pointer object.

> +          (ret (proc ifapp))
> +          (err (errno)))
> +     (if (zero? ret)
> +         (next-ifaddr (parse-ifaddrs ifapp))

Use ‘read-ifaddrs’ instead of ‘parse-ifaddrs’.

> +(define (getifaddrs)
> +  "Obtain a list of network interfaces on the local system."

s/Obtain a/Return the/

> +  (let ((ifaddrs (%getifaddrs)))
> +    (let lp ((curr ifaddrs) (res '()))
> +      (if (last-interface? curr)
> +       (reverse res)
> +       (lp (next-ifaddr curr) (cons curr res))))))

s/lp/loop/ for consistency

This is OK, but the problem is that each object in the list that is
returned is a tuple, so it’s not very convenient.

What about defining a <interface-address> record type, and converting
the tuples to that, so that users of ‘getifaddrs’ directly get this more
convenient interface?  Like:

  (define-record-type <interface-address>
    (interface-address name flags)
    interface-address?
    (name   interface-address-name)
    (flags  interface-address-flags))

The type predicate and accessors need to be exported, of course.

> +;; Given a pointer to a struct ifaddrs, parse it into a list.
> +(define-syntax-rule (parse-ifaddrs ptr)
> +  (parse-c-struct ptr %struct-ifaddrs-type))

No longer needed.

> +;; Retrieve a bytevector aliasing the memory pointed to by the
> +;; ifa_next struct ifaddrs* pointer.
> +(define-syntax-rule (next-ifaddr ifaddrs)
> +  (parse-c-struct (car ifaddrs) %struct-ifaddrs-type))

s/define-syntax-rule/define/

Use ‘match’ instead of ‘car’; same for the following macros.

> +;; Retrieve interface name.
> +(define-syntax-rule (ifaddr-name ifaddrs)
> +  (pointer->string (cadr ifaddrs)))
> +
> +;; Retrieve interface flags.
> +(define-syntax-rule (ifaddr-flags ifaddrs)
> +  (list-ref ifaddrs 2))

These become unneeded once <interface-address> is added.

Could you send an updated patch?

If something is unclear, please let me know!

Thank you!

Ludo’.