Added support for vsock: target URI scheme#24551
Added support for vsock: target URI scheme#24551egranata wants to merge 1 commit intogrpc:masterfrom egranata:vsock
Conversation
markdroth
left a comment
There was a problem hiding this comment.
I'm not sure why you opened a new PR here; it would have been better to just merge master into #21745 so that we didn't lose the active comment threads. Many of my original comments still need to be addressed, so I've copied them here.
Also, is there a reasonable way to test this new code? I'm a little reticent to accept code that we can't actually verify is working.
Please let me know if you have any questions. Thanks!
Reviewed 29 of 29 files at r1.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @egranata)
src/core/lib/iomgr/parse_address.cc, line 85 at r1 (raw file):
return false; }
Please remove blank lines within functions.
src/core/lib/iomgr/resolve_address_posix.cc, line 60 at r1 (raw file):
} if (name[0] == 'v' && name[1] == 's' && name[2] == 'o' && name[3] == 'c' &&
Copying my comment here from #21745:
Please remove this. The code we have above this for "unix:" URIs is a really ugly hack that I've been meaning to get rid of. And even though I haven't yet gotten around to doing that, I don't want to repeat the existing mistake here.
The iomgr name resolution code is used both on the client and on the server, but the ability for it to understand "unix:" or "vsock:" names is only needed on the server side, because the client side only uses this code when it already knows it's a DNS name -- if you specify a "unix:" or "vsock:" URI on the client side, it selects a different resolver that does not call the iomgr name resolution code in the first place.
So instead of adding this hack here, we should instead move this check into the server code, which is the only place where it's actually needed.
For bonus points, feel free to move the existing "unix:" code at the same time.
src/core/lib/iomgr/resolve_vsock_fallback.cc, line 1 at r1 (raw file):
/*
Please use C++-style comments.
src/core/lib/iomgr/resolve_vsock_linux.cc, line 1 at r1 (raw file):
/*
Please use C++-style comments.
src/core/lib/iomgr/resolve_vsock_linux.cc, line 42 at r1 (raw file):
grpc_error* grpc_resolve_vsock_address(const char* name, grpc_resolved_addresses** addrs) { struct sockaddr_vm *vm;
Please declare this where it's used.
src/core/lib/iomgr/resolve_vsock_linux.cc, line 45 at r1 (raw file):
unsigned int cid; unsigned int port;
Please remove blank lines within functions.
Same thing throughout.
src/core/lib/iomgr/resolve_vsock_linux.cc, line 55 at r1 (raw file):
(*addrs)->addrs = static_cast<grpc_resolved_address*>( gpr_zalloc(sizeof(grpc_resolved_address))); vm = (struct sockaddr_vm *)(*addrs)->addrs->addr;
Please use C++-style casts.
src/core/lib/iomgr/resolve_vsock_linux.cc, line 79 at r1 (raw file):
char *result; struct sockaddr_vm *vm = (struct sockaddr_vm*)addr;
Please use C++-style casts.
src/core/lib/iomgr/sockaddr_utils.cc, line 271 at r1 (raw file):
return grpc_ntohs(((grpc_sockaddr_in6*)addr)->sin6_port); case GRPC_AF_VSOCK: #ifdef GRPC_HAVE_LINUX_VSOCK
Copying my comment from #21745:
This ifdef probably is needed, but it would be better not to bury it inside of this file. Instead, I suggest defining a function called something like grpc_vsock_addr_get_port() with a real definition in vsock.cc and a no-op definition in novsock.cc, and then unconditionally call that function here.
src/core/lib/iomgr/sockaddr_utils.cc, line 304 at r1 (raw file):
return 1; case GRPC_AF_VSOCK: #ifdef GRPC_HAVE_LINUX_VSOCK
Similar comment here.
src/core/lib/iomgr/unix_sockets_posix.h, line 50 at r1 (raw file):
const grpc_resolved_address* resolved_addr); char* grpc_sockaddr_to_uri_vsock_if_possible(
This should return a std::string.
|
This issue/PR has been automatically marked as stale because it has not had any update (including commits, comments, labels, milestones, etc) for 30 days. It will be closed automatically if no further update occurs in 7 day. Thank you for your contributions! |
This is another attempt to add support for vsock in grpc since previous PRs(#24551, #21745) all closed without merging. The VSOCK address family facilitates communication between virtual machines and the host they are running on. This patch will introduce new scheme: [vsock:cid:port] to support VSOCK address family. Fixes #32738. --------- Signed-off-by: Yadong Qi <yadong.qi@intel.com> Co-authored-by: AJ Heller <hork@google.com> Co-authored-by: YadongQi <YadongQi@users.noreply.github.com>
This is another attempt to add support for vsock in grpc since previous PRs(grpc#24551, grpc#21745) all closed without merging. The VSOCK address family facilitates communication between virtual machines and the host they are running on. This patch will introduce new scheme: [vsock:cid:port] to support VSOCK address family. Fixes grpc#32738. --------- Signed-off-by: Yadong Qi <yadong.qi@intel.com> Co-authored-by: AJ Heller <hork@google.com> Co-authored-by: YadongQi <YadongQi@users.noreply.github.com>
This is another attempt to add support for vsock in grpc since previous PRs(grpc#24551, grpc#21745) all closed without merging. The VSOCK address family facilitates communication between virtual machines and the host they are running on. This patch will introduce new scheme: [vsock:cid:port] to support VSOCK address family. Fixes grpc#32738. --------- Signed-off-by: Yadong Qi <yadong.qi@intel.com> Co-authored-by: AJ Heller <hork@google.com> Co-authored-by: YadongQi <YadongQi@users.noreply.github.com>
@markdroth