[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] getifaddrs wrapper
From: |
Rohan Prinja |
Subject: |
Re: [PATCH] getifaddrs wrapper |
Date: |
Thu, 2 Jul 2015 16:29:26 +0530 |
PTAL, tests to follow soon.
Thank you.
On 19 June 2015 at 17:33, Ludovic Courtès <address@hidden> wrote:
> 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’.
0001-guix-build-syscalls.scm-refactor-according-to-code-r.patch
Description: Text Data
- Re: [PATCH] getifaddrs wrapper,
Rohan Prinja <=