nodejs / node-v0.x-archive

Moved to https://github.com/nodejs/node
34.43k stars 7.31k forks source link

Can't get datagram fd without referencing internal API #8057

Closed benjamincburns closed 1 year ago

benjamincburns commented 10 years ago

When I open a datagram socket using dgram.createSocket('udp4'), I'd expect the fd member of the socket to be set to the socket's fd once it's been properly opened. However, it's never set there, and in order to get the socket's fd, I have to snoop on the private member socket._handle.fd.

Why is this important to me?

I'm working on a module to implement support for TUNTAP network interfaces as an enabler of node-based virtual networks (think VPNs and VM networks). As a personal goal I'm attempting to write this module with no native code, and it's almost doable by using the ref and ioctl modules.

I say almost because while I'm able to create the TUN/TAP device, I'm not able to get/set options on it (address, broadcast, netmask, mtu) without referencing the private datagram socket API (_handle).

Example code:

TunTap.prototype.setAddress = function(addr) {
    var ifr = new ifreq();
    ifr.ref().fill(0);

    // the structure of ifreq is really ugly without the C preprocessor :-(
    ifr.ifrn_name.buffer.write(this.name);
    ifr.ifr_ifru.ifru_addr.sockaddr_in.sin_family = AF_INET;
    ifr.ifr_ifru.ifru_addr.sockaddr_in.sin_addr = inet_aton(addr);

    var socket = dgram.createSocket('udp4');

    // socket isn't open yet, so we bind it to let it open
    socket.bind();
    socket.on('listening', function() {
        try {
            //////// vvv PROBLEM IS HERE vvv ////////
            var fd = socket.fd;
            if (fd == null || fd < 0 ) { // fd is -42 here for some reason...
                fd = socket._handle.fd;
            }
            //////// ^^^ PROBLEM IS HERE ^^^ ////////
            ioctl(fd, SIOCSIFADDR, ifr.ref());
            this.emit('addr', addr);
        } catch (err) {
            this.emit('error', 'error setting address on device ' + this.name +
                    ' to ' + addr + ' due to error: ' + err);
        }
        socket.close();
    }.bind(this));
}
benjamincburns commented 10 years ago

I'd be very happy to submit a PR which updates dgram.Socket.fd to dgram.Socket._handle.fd when the latter is numeric and greater than or equal to 0. However, the comments in the code here and here and here lead me to believe that this will cause problems. Would be happy to "unhack" this if necessary, but I think I'd need some direction as to what's going on here.

Perhaps since dgram is considered stable we could expose the internal _handle member as handle? I'd also be happy to submit a PR for this, along with some supporting documentation for this member.

trevnorris commented 10 years ago

I think it would be better to make dgram.Socket#fd a getter. That way in the future if we change the underlying _handle the getter can also be updated.

@indutny What do you think?

BTW, "stable" simply means the user API. Not the internal implementation.

benjamincburns commented 10 years ago

If this issue is addressed, I'd be quite pleased if the resultant API was consistent across all of the various stream-like classes in node's js runtime. Ideally I don't think it's too unreasonable to say that if a class makes use of an fd, is stream-like, and is part of the node API, I shouldn't need any instanceof or typeof or hasOwnProperty checks to interact with it (including when I need to get the underlying fd).

It strikes me that classes which fit those criteria would all inherit from some common subclass, anyway - perhaps fs.XXXStream? Has any effort or consideration toward this kind of generalization been made?

saghul commented 10 years ago

I think it would be better to make dgram.Socket#fd a getter. That way in the future if we change the underlying _handle the getter can also be updated.

FWIW, the function that currently gets the fd (https://github.com/joyent/node/blob/master/src/stream_wrap.cc#L70) replies on non-public libuv implementation details.

benjamincburns commented 10 years ago

@saghul - sounds like more fodder for the discussion over at #8002. Is there a similar issue over in libuv-land that I can follow? If not, since I think you've got some goals in mind, would you mind raising one? Will be very happy to contribute in any way that I can.

saghul commented 10 years ago

@benjamincburns yeah, it's indeed related. Currently there is no libuv issue opened for this, and we have resisted those which wanted to expose the fd, because it's treated as an internal implementation detail. There are ways around it, but not for all cases, so we'll get to come up with something.

I don't have any concrete suggestions as of now (well, the obvious uv_get_fd, I guess), but please do open an issue on libuv so we can discuss this further.

trevnorris commented 10 years ago

@benjamincburns If you do, please link back to this issue for dependency tracking.

saghul commented 10 years ago

FYI: https://github.com/joyent/libuv/pull/1435