From: ludo@gnu.org (Ludovic Courtès)
To: Rohan Prinja <rohan.prinja@gmail.com>
Cc: guix-devel <guix-devel@gnu.org>
Subject: Re: [PATCH] getifaddrs wrapper
Date: Fri, 19 Jun 2015 14:03:14 +0200 [thread overview]
Message-ID: <87mvzvykfx.fsf@gnu.org> (raw)
In-Reply-To: <CAAT4Lc4jN+YdYnX_09gxB5UL_d+QiLWhG07GEqVR6rSL1Uh0AQ@mail.gmail.com> (Rohan Prinja's message of "Fri, 19 Jun 2015 15:20:16 +0530")
Rohan Prinja <rohan.prinja@gmail.com> skribis:
> From 668910afbe979145a7699708817e28d219ec0750 Mon Sep 17 00:00:00 2001
> From: Rohan Prinja <rohan.prinja@gmail.com>
> 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’.
next prev parent reply other threads:[~2015-06-19 12:03 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-06-19 9:50 [PATCH] getifaddrs wrapper Rohan Prinja
2015-06-19 12:03 ` Ludovic Courtès [this message]
2015-07-02 10:59 ` Rohan Prinja
2015-07-02 12:23 ` Ludovic Courtès
2015-07-16 8:30 ` Rohan Prinja
2015-07-16 13:50 ` Rohan Prinja
2015-07-16 15:38 ` Ludovic Courtès
2015-07-17 10:57 ` Rohan Prinja
2015-07-25 12:59 ` Ludovic Courtès
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87mvzvykfx.fsf@gnu.org \
--to=ludo@gnu.org \
--cc=guix-devel@gnu.org \
--cc=rohan.prinja@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/guix.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.