[libvirt] [PATCH 06/10] Add a generic internal API for handling any FD based stream

[Eric Blake]

On 11/02/2010 05:42 AM, Daniel P. Berrange wrote:
>>> -    VIR_FROM_AUDIT      /* Error from auditing subsystem */
>>> +    VIR_FROM_AUDIT,     /* Error from auditing subsystem */
>>> +    VIR_FROM_STREAMS,   /* Error from I/O streams */
>>>  } virErrorDomain;
>>
>> Is the switch from C89 style (no trailing comma) to the C99 style
>> (optional trailing comma permitted) intentional?  Personally, I like it,
>> since adding a new value at the end no longer requires a random-looking
>> diff of the previous entry just to add a comma.
> 
> IMHO, leaving off the comma needlessly enlarges future patches 
> because 2 lines have to be changed to add an entry instead 
> of just one.

Sounds like we're in violent agreement here, about favoring C99 syntax.

Is it worth a generic cleanup patch for other enum declarations that
don't yet use trailing commas, or should we just save it for an
as-encountered basis?

>>> +#include <sys/un.h>
>>
>> but this is missing on Mingw, with no gnulib replacement as of yet.  Do
>> we need to add some HAVE_SYS_UN_H checks?
> 
> Yes, i guess so.
> 
>>
>>> +#include <netinet/in.h>
>>> +#include <netinet/tcp.h>
>>
>> Likewise for HAVE_NETINET_TCP_H.
> 
> Doesn't gnulib take care of TCP socket portability for us ?

So far, gnulib provides <netinet/in.h>, but not <netinet/tcp.h>.  But
what exactly are we using that's only in netinet/tcp.h?  It may be easy
to port to gnulib, since mingw does have tcp support (sys/un.h is the
much harder task, since mingw has no notion of unix sockets).

>>> +static int virFDStreamWrite(virStreamPtr st, const char *bytes, size_t nbytes)
>>
>> Should this return ssize_t...
> 
> No, this has to return an int, to match the public API calling
> convention.

Then we have to check for overflow, and explicitly return an error if
nbytes > INT_MAX.

>>> +    if ((st->flags & VIR_STREAM_NONBLOCK) &&
>>> +        (!S_ISCHR(sb.st_mode) &&
>>> +         !S_ISFIFO(sb.st_mode))) {
>>
>> Should we also permit S_ISSOCK as an fd that supports reliable
>> non-blocking behavior?
> 
> AFAIK, there's no way to open a socket as a path on the
> filesystem is there ?  So there'd be no way that open(path)
> could return an FD for which S_ISSOCK() is true.

You're probably right that it's not possible to bind a socket to a
standard file system.  But it is possible via procfs (think
/proc/nnn/fd/nnn of a process which already has a socket open), and
there might also be some magic /dev/ or /sys/ mappings that can provide
a socket (at any rate, bash provides magic handling of
/dev/tcp/host/port, whether or not the kernel also has a magic
filesystem to provide that support automatically).  So it boils down to
a question of whether we want to permit or reject sockets, on the
pre-condition that the user finds a way to hand us a filename that
resolves to a socket.

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20101102/af49b305/attachment-0001.sig>