Skip to content
Closed
149 changes: 146 additions & 3 deletions include/envoy/network/io_handle.h
Original file line number Diff line number Diff line change
@@ -1,33 +1,176 @@
#pragma once

#include <bitset>
#include <memory>

#include "envoy/common/pure.h"

namespace Envoy {

namespace Buffer {
Copy link
Member

Choose a reason for hiding this comment

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

nit: prefer include header vs. forward decl unless there is a circular dependency, same below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

include/envoy/buffer/buffer.h will depend on io_handle.h, so will network/address.h.

struct RawSlice;
} // namespace Buffer

namespace Network {

namespace Address {
class Instance;
} // namespace Address

class IoError;

// IoErrorCode::Again is used frequently. Define it to be a distinguishable address to avoid
// frequent memory allocation of IoError instance.
// If this is used, IoHandleCallResult has to be instantiated with a deleter that does not
// deallocate memory for this error.
#define ENVOY_ERROR_AGAIN reinterpret_cast<IoError*>(0x01)

class IoError {
public:
enum class IoErrorCode {
Copy link
Contributor

Choose a reason for hiding this comment

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

if this merges after #5772, can we also update that API to reference IOError here rather than do what it's doing?

Also IMO IOError should not be underneath the Network namespace, as this also makes sense for non-networked I/O.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR won't be checked in as a whole. Otherwise, all the implementations of these interfaces will have to be there and replacing all the direct sys call's to fd. I plan to check in the methods one by one in following PR's, starting from #5171 which will very likely has IoErrorCode definition.

Where would you like to place IoError?
And it will be great if #5772 can have TODO for switching to IoError if it's checked in first.

// No data available right now, try again later.
Again,
// Not supported.
NoSupport,
// Address family not supported.
AddressFamilyNoSupport,
// During non-blocking connect, the connection cannot be completed immediately.
InProgress,
// Permission denied.
Permission,
// Other error codes cannot be mapped to any one above in getErrorCode().
UnknownError
};
virtual ~IoError() {}

// Map platform specific error into IoErrorCode.
// Needed to hide errorCode() in case of ENVOY_ERROR_AGAIN.
static IoErrorCode getErrorCode(const IoError& err) {
if (&err == ENVOY_ERROR_AGAIN) {
return IoErrorCode::Again;
}
return err.errorCode();
}

static std::string getErrorDetails(const IoError& err) {
if (&err == ENVOY_ERROR_AGAIN) {
return "Try again later";
}
return err.errorDetails();
}

protected:
// Map platform specific error into IoErrorCode.
virtual IoErrorCode getErrorCode() const PURE;

virtual std::string getErrorDetails() const PURE;
};

using IoErrorDeleterType = void (*)(IoError*);
using IoErrorPtr = std::unique_ptr<IoError, IoErrorDeleterType>;

/**
* Basic type for return result which has a return code and error code defined
* according to different implementations.
* If the call succeeds, |err_| is nullptr and |rc_| is valid. Otherwise |err_|
* can be passed into IoError::getErrorCode() to extract the error. In this
* case, |rc_| is invalid.
*/
template <typename T> struct IoHandleCallResult {
IoHandleCallResult(T rc, IoErrorPtr err) : rc_(rc), err_(std::move(err)) {}
virtual ~IoHandleCallResult() {}

T rc_;
IoErrorPtr err_;
};

using IoHandleCallIntResult = IoHandleCallResult<int>;
using IoHandleCallSizeResult = IoHandleCallResult<ssize_t>;

/**
* IoHandle: an abstract interface for all I/O operations
*/
class IoHandle {
public:
IoHandle() {}
enum class ShutdownType { Read = 0, Write, Both };

enum class IoHandleFlag {
// Each of these maps to a unique bit.
NonBlock = 0b01,
Append = 0b10
};

virtual ~IoHandle() {}

/**
* Return data associated with IoHandle.
*
* TODO(sbelair2) remove fd() method
* TODO(danzh) move it to IoSocketHandle after replacing the calls to it with
* calls to IoHandle API's everywhere.
*/
Copy link
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
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
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
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
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
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
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(Buffer::RawSlice* iovecs, int num_iovec) PURE;

virtual IoHandleCallIntResult recvmmsg(struct mmsghdr* msgvec, unsigned int vlen, int flags,
struct timespec* timeout) PURE;

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

virtual IoHandleCallIntResult sendmmsg(struct mmsghdr* msgvec, unsigned int vlen, int flags) PURE;

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

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

Copy link
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
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
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
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
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
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 Address::addressFromFd()
*/
virtual IoHandleCallIntResult getBindToAddress(Network::Address::Instance** address) PURE;

/**
* Wrap Address::peerAddressFromFd()
*/
virtual IoHandleCallIntResult getPeerAddress(Network::Address::Instance** address) PURE;

/**
* Wrap fcntl(fd_, F_SETFL...)
* @param flag each bit stands for a flag in enum IoHandleFlag.
*/
virtual IoHandleCallIntResult setIoHandleFlag(std::bitset<2> flag) PURE;
Copy link
Member

Choose a reason for hiding this comment

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

Can we document what the bitset actually means? Potentially with an enum of each bit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Member

Choose a reason for hiding this comment

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

I think you can just refer back to the enum which will have 0x1, etc. for docs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

/**
* Wrap fcntl(fd_, F_GETFL...)
*/
virtual IoHandleCallResult<std::bitset<2>> getIoHandleFlags() PURE;

virtual IoHandleCallIntResult listen(int backlog) PURE;

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

virtual IoHandleCallIntResult shutdown(ShutdownType how) PURE;
};
Copy link
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
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
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
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
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