Skip to content

Added support for vsock: target URI scheme#24551

Closed
egranata wants to merge 1 commit intogrpc:masterfrom
egranata:vsock
Closed

Added support for vsock: target URI scheme#24551
egranata wants to merge 1 commit intogrpc:masterfrom
egranata:vsock

Conversation

@egranata
Copy link
Copy Markdown

@markdroth markdroth self-assigned this Oct 26, 2020
Copy link
Copy Markdown
Member

@markdroth markdroth left a comment

Choose a reason for hiding this comment

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

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.

@stale
Copy link
Copy Markdown

stale bot commented Jan 25, 2021

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!

@stale stale bot closed this Feb 2, 2021
drfloob added a commit that referenced this pull request May 30, 2023
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>
eugeneo pushed a commit to eugeneo/grpc that referenced this pull request Jun 1, 2023
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>
mario-vimal pushed a commit to mario-vimal/grpc that referenced this pull request Jun 15, 2023
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants