Hi Brandon -
The big question - is do we like the Go-ish tuple return
style in these functions? Should other I/O routines also
work that way? I think it would be a real benefit to
Chapel's programmability if we make an effort to be
consistent in the module APIs...
I would prefer to consolidate the error returns
into optional arguments and a single return type. For
example, instead of
(socket, err) = listen("tcp, "", port)
we could fold an error code into our 'socket' class,
so that you can check the error if you want to, and
operations on a error-ful socket will continue returning
the error and/or raise an exception/halt. (I believe
this is already the case with file and channel, but
I might be wrong).
Then in
for (conn, ip, err) in socket do begin { ...
there is no real reason why we can't get the peer IP
out of the connection, right? So if the connection contained
both an IP and an error code, you could return:
for conn in socket do begin ...
and use conn.error and conn.peer.
Lastly, in the I/O modules I chose to use records with reference
counting. The reference counting is currently being improved
(you noticed that the last channel with an open file did
not cause it to close, but that should work eventually). Anyway,
if socket is of type class Listener, not only would you have to
do socket.close() but also delete socket; (since Chapel is not
currently garbage collected). Have a look at
the channel or file records in IO.chpl - I think it would make
sense to mirror their reference-counted-ness. Lastly, since
Chapel is a distributed language, your Listener should function
correctly when nodes on another locale use it. (Or at least
detect that case and cause an error). As it is, a remote locale
would try to use an fd number that wasn't open on that locale.
Since your Listener is really a 'file' that creates other 'files',
it wouldn't bother me if you jam the functionality into the
file type. Note that - as you did with the string type - you
can define methods on the file type in the Net module that
will only be available if you use Net;.
More details below.
>> - extern proc sys_freeaddrinfo(res:sys_addrinfo_ptr_t);
>> + extern proc sys_freeaddrinfo(inout res:sys_addrinfo_ptr_t);
>> I don't think the inout belongs there, since sys_freeaddrinfo takes in a
>> pointer (not pointer to pointer),
>> and you didn't change the prototype in sys.h other than to remove an _...
> The sys_freeaddrinfo implementation that was already there took a
> sys_addrinfo_ptr_t* and set the referent to NULL. I assume the reason is to
> keep dangling pointers from leaking into Chapel code. I decided to make the
> Sys.chpl prototype match the sys.c implementation instead of vice versa.
That's odd. I think it would be better to match the C API; less to think about.
Of course I probably wrote the set-ptr-to-null business...
>> In qio.c
>> - if( src_addr_out ) src_addr_out->len = msg.msg_namelen;
>> Why are you removing the setting of the length here? ...
> Oh, yes, I forgot to consider storage for named pipe addresses....
So, I did a little bit of research... I think your changes are OK:
- a 'unix domain socket' (the address family with variable-length addresses)
has a pathname in it, but struct sockaddr_storage
must have enough room to store it
- the pathname is null-terminated
- I see other examples of always passing sizeof(variable) to
connect/accept/etc;
so I believe that always passing sizeof(struct sockaddr_storage) in our case
would be correct. (You just have to make a local variable in the cases
like accept that need a pointer...)
However, I can't see anywhere in the standard documentation that it's
always OK to allocate more space than necessary and pass in extra length
to e.g. connect.
Details of review of Net.chpl:
What is this?
+use _InternalDecor;
Oh - I see later
+// This is a small decorator module used internally to add useful methods to
+// various types without making them available to users.
+module _InternalDecor ...
These look like really useful routines. Couldn't you add them to the standard
module where the other String routines reside? Also, we should try to be
consistent in argument order, etc, between string split and regular
expression split (which is in module Regexp.chpl).
What's going on here?
+proc listen(tuple:(string, Addr)) { // sigh...
+ return listen(tuple(1), tuple(2));
+}
+ // We should really pass null to getaddrinfo, but that's annoying in Chapel.
+ // We can handle it later.
+ if host == "" then host = "127.0.0.1";
You should be able to pass 'nil' in if you declare getaddrinfo in the
right way (possibly including leaving the ref hints:sys_addrinfo_t
as un-typed like hints). You should be able to declare two versions
of the extern proc with the same name (for type overloading).
------------------------------------------------------------------------------
Rapidly troubleshoot problems before they affect your business. Most IT
organizations don't have a clear picture of how application performance
affects their revenue. With AppDynamics, you get 100% visibility into your
Java,.NET, & PHP application. Start your 15-day FREE TRIAL of AppDynamics Pro!
http://pubads.g.doubleclick.net/gampad/clk?id=84349831&iu=/4140/ostg.clktrk
_______________________________________________
Chapel-developers mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/chapel-developers
