[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] 1/3 Add IPv4 and IPv6 parsing routines



On Wed, Oct 14, 2009 at 10:49:20AM +0100, Daniel P. Berrange wrote:
> On Tue, Oct 13, 2009 at 04:12:47PM +0200, Daniel Veillard wrote:
> >     * src/util/util.h src/util/util.c: two new functions virParseIPv4
> >       and virParseIPv6
> 
> I think this should just be a thin wrapper around getaddrinfo()
> which already knows how to parse all possible address types.

  Are we allowing all possible address types in the XML ?
I based that parsing routing in a large part as a syntactic check
on the allowed set of addresses. For example I didn't allow

   ::ffff:12.34.56.78

kind of IPv6 syntax.

> If we avoid a custom typedef, and just use 'struct sockaddr_storage'
> this in turn makes it easy for callers to pass the result straight
> into any socket API calls they might use. eg this could all be done
> with a couple of lines of code
> 
>   int virSocketAddr(const char *str, struct sockaddr_storage *ss)
>   {

  The point of the exercise was to make some checks on the addresses
given as string and passed down as string to an external tool

>     int len;
>     struct addrinfo hints = { 0 };
>     struct addrinfo *res = NULL;
> 
>     hints.ai_flags = AI_NUMERICHOST;
> 
>     if (getaddrinfo(str, NULL, &hints, &res) != 0 || !res)
>       return -1;
> 
>     len = res->addrlen;
>     memcpy(ss, res->ai_addr, len);
> 
>     freeaddrinfo(res);
>     return len;
>   }
> 
> That would automatically cope with both IPv4 / 6 addresses. If
> we want to restrict it we could add a 3rd argument, 'int family'
> and use that to set  hints.ai_family field - the caller would
> just pass AF_INET or AF_INET6 to specify a particular type, or
> leave it at 0 to allow any type.

  Dunno, I find the getaddrinfo interface and the hints stuff
fairly incomprehensible. When I see the OpenGroup definition of
struct sockaddr_storage

struct sockaddr_storage {
    sa_family_t  ss_family;  /* Address family. */
/*
 *  Following fields are implementation-defined.
 */
    char _ss_pad1[_SS_PAD1SIZE];
        /* 6-byte pad; this is to make implementation-defined
           pad up to alignment field that follows explicit in
           the data structure. */
    int64_t _ss_align;  /* Field to force desired structure
                           storage alignment. */
    char _ss_pad2[_SS_PAD2SIZE];
        /* 112-byte pad to achieve desired size,
           _SS_MAXSIZE value minus size of ss_family
           __ss_pad1, __ss_align fields is 112. */
};

 I'm not too enthusiastic about using this for internal APIs.
And I don't see how I would check ranges for the IP addresses based
on this. Actually I don't find it helps to calculate a range,
and I prefer my good old arrays of well defined ints for that purpose.

> It is probably worth introducing a 'src/util/socketaddr.h' file
> for this, since I think we'll need more socket addr functions
> later

  No idea you had that in mind, go for it !

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel veillard com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]