Skip to content
72 changes: 69 additions & 3 deletions include/envoy/network/io_handle.h
Original file line number Diff line number Diff line change
Expand Up @@ -7,27 +7,93 @@
namespace Envoy {
namespace Network {

namespace Address {
class Instance;
} // namespace Address

/**
* Basic type for return result which has a return code and error code defined
* according to different implementation.
*/
template <typename T> struct IoHandleCallResult {
virtual ~IoHandleCallResult() {}

T rc_;
int errno_;
Comment thread
danzh2010 marked this conversation as resolved.
Outdated
};

typedef IoHandleCallResult<int> IoHandleCallIntResult;
typedef IoHandleCallResult<ssize_t> IoHandleCallSizeResult;
Comment thread
danzh2010 marked this conversation as resolved.
Outdated

/**
* IoHandle: an abstract interface for all I/O operations
*/
class IoHandle {
public:
IoHandle() {}

virtual ~IoHandle() {}

/**
* Return data associated with IoHandle.
*
* TODO(sbelair2) remove fd() method
* We probably still need some method similar to this one for
* evconnlistener_new(). Or We can move it to IoSocketHandle and down cast the
* IoHandle to IoSocketHandle wherever needed.
*/

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My opinion is that it would be cleaner to move this to the derived IoSocketHandle- what does the fd() method resolve to for interfaces that are not sockets? Either that, or there can be a generic method to manage a descriptor of sorts, such that for sockets, the descriptor is really just the fd. And for other interfaces it might be a session id, etc.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer a generic method to return some sorts of identity which can be used differently for different implementation. WDYT?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danzh1989 @sbelair2 I don't think I'm grokking this convo fully. Can one or both of you expand? I don't think we should still need this method so I'm likely missing something.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mattklein123 Maybe I missed some earlier conversation, but what shall be passed into evconnlistener_new() if there is no fd() or something similar in interface?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could probably figure out an interface strategy in which the socket actually creates the listener somehow, but for now, I would probably just do a dynamic_cast. It's a little ugly, but VPP or similar will have to have their own listener implementation, so it should fit together OK? Thoughts?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mattklein123 Do you mean to make fd() a method to IoSocketHandle only and dynamic_cast the IoHhandle object when we create listener? That sounds reasonable to me.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that's what I meant. IMO that's OK for now.

virtual int fd() const PURE;

/**
* Clean up IoHandle resources
*/
virtual void close() PURE;
virtual IoHandleCallIntResult close() PURE;

virtual bool isClosed() PURE;

virtual IoHandleCallSizeResult readv(const iovec* iovec, int num_iovec) PURE;
Comment thread
danzh2010 marked this conversation as resolved.
Outdated

virtual IoHandleCallSizeResult writev(const iovec* iovec, int num_iovec) PURE;

virtual IoHandleCallIntResult bind(const Network::Address::Instance& address) PURE;

virtual IoHandleCallIntResult connect(const Network::Address::Instance& server_address) PURE;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For UDP we will need methods that provide the equivalent of sendmmsg and recvmmsg.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added. Not sure if envoy has something equivalent to mmsghdr or msghdr.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not aware of any equivalent currently--the closest I can think of would be the UdpData struct @conqerAtapple added recently. Though if we wanted to use that, we would have to add analogues for msghdr.msg_control and msghdr.msg_flags fields. @conqerAtapple , do you have an opinion?

/**
* Wrap setsockopt()
*/
virtual IoHandleCallIntResult setIoHandleOption(int level, int optname, const void* optval,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest we use an enum for the options we care about. The one gotcha here is IIRC we have some capability in the config to actually set options that the code doesn't know about, by int, so maybe this isn't possible and we need to keep this? Thoughts?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would there be potential crash if envoy fails to map the value set in config and the enum defined here? If so, introducing enum for socket option can be error-prone.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right I think if we have a config option to set an int, we need to leave it as is.

socklen_t optlen) PURE;
/**
* Wrap getsockopt()
*/
virtual IoHandleCallIntResult getIoHandleOption(int level, int optname, void* optval,
socklen_t* optlen) PURE;

/**
* Wrap getsockname()
*/
virtual IoHandleCallIntResult getIoHandleName(const Network::Address::Instance& address) PURE;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: how will getIoHandleName() and getPeerName() actually return the name? They take const params, whereas getsockname() takes a mutable out-param. (Though making the Network::Address::Instance param non-const may not be sufficient, since Address::Instance isn't mutable in a way that allows it to be changed to a different address.)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah we will need some type of out param for these functions. You could either do an out param is an actual parameter, or have it return a struct of a result code and the address. I actually prefer the latter but I don't feel strongly about it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer an output param of Network::Address::Instance type.


virtual IoHandleCallIntResult getPeerName(const Network::Address::Instance& address) PURE;

/**
* Wrap fcntl(fd_, F_SETFL...)
*/
virtual IoHandleCallIntResult setIoHandleFlag(int flag) PURE;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

enum for options we care about?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added enum IoHandleFlag, but we only used O_NONBLOCK so far.

/**
* Wrap fcntl(fd_, F_GETFL...)
*/
virtual IoHandleCallIntResult getIoHandleFlag() PURE;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Return a bit set of enums that we care about?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you mean std::bitset or just int?


virtual IoHandleCallIntResult listen(int backlog) PURE;

/**
* Wrap dup()
*/
virtual std::unique_ptr<IoHandle> dup() PURE;

virtual IoHandleCallIntResult shutdown(int how) PURE;
Comment thread
danzh2010 marked this conversation as resolved.
Outdated
};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to me that there is probably no point in defining the template class IoHandleCallResult if all the virtual methods in the base class are all IoHandleCallResult or IoHandleCallResult<ssize_t>. I think we need to examine if there are any return values that can be other than integer types. If there are other interfaces that need this (I'm not saying that VPP is one of them), then won't these virtual methods preclude them?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danzh2010 I do want to say that I like the direction where this is going.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you concerned about some implementations of same method whose return a type is different from what is declared here? For such method, I would say same purpose can be achieved by adding another new method to the interface.

If IoHandleCallResult and IoHandleCallResult<ssize_t> are the only two return types we need, what would you like the IoHandleCallResult to be without template?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danzh2010 Sorry, forgot about this. I'm only thinking that we should not preclude further options, but maybe that's just too much caution? Right now I cannot think of other return types we might need, but that doesn't mean that such a need won't come up later.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now we have IoHandleCallResult<std::bitset<2>>


typedef std::unique_ptr<IoHandle> IoHandlePtr;

} // namespace Network
Expand Down