The committers are authorized under a signed CLA.

• ✅ Enrico Granata (b4fd529)

@ejona86 is this sort of naming scheme something we want to keep promoting? This is adding vsock: as a naming scheme here.

Also as a general code style, we try as much as possible to avoid the paradigms of:

void doSomething() {
#ifdef X
  doSomethingWithX();
#else
  doAFallback();
#endif
}

And rather have whole files dedicated to features being enabled / disabled. In this case, this would mean having a vsock.cc file that contains all the vsock-enabled function, with a global #ifdef / #endif around it, and a novsock.cc file that contains all the opposites.

vsock: prefix is fine. Although because that is an address, it would be most similar to ip:, inprocess:, unix:, which are something a bit more akin to convenience.

Now the format appears to be vsock:cid:port. I don't know if that is the format we want. But using the scheme vsock seems fine. (FWIW, Java today doesn't support any of the schemes I've mentioned here. I think Go only supports unix:. Java would like to support inprocess and unix, though.)

vsock: prefix is fine. Although because that is an address, it would be most similar to ip:, inprocess:, unix:, which are something a bit more akin to convenience.

Now the format appears to be vsock:cid:port. I don't know if that is the format we want. But using the scheme vsock seems fine. (FWIW, Java today doesn't support any of the schemes I've mentioned here. I think Go only supports unix:. Java would like to support inprocess and unix, though.)

I think we are definitely open to suggestions on the format of the name. What do you think would work better than vsock:cid:port here?

FWIW, Chrome OS is adding vsock support to libiio/iiod and they are going with vsock:port (the CID is implied to be the HOST in that case), see https://chromium-review.googlesource.com/c/chromiumos/third_party/libiio/+/1719570.

YMMV wrt how much that matters since I was previously involved with that work, so make of that choice what you will

Also as a general code style, we try as much as possible to avoid the paradigms of:

void doSomething() {
#ifdef X
  doSomethingWithX();
#else
  doAFallback();
#endif
}

And rather have whole files dedicated to features being enabled / disabled. In this case, this would mean having a vsock.cc file that contains all the vsock-enabled function, with a global #ifdef / #endif around it, and a novsock.cc file that contains all the opposites.

Got it. I can certainly work on that.

I have taken the feedback into account and moved several of the #ifdef blocks into separate files. There's a few smaller sections of conditional code that I left as-is, just because they are one or two lines each.

Please do take a look when you get a chance.

Thanks all for the feedback so far.

We can't look at this until you sign the CLA.

We can't look at this until you sign the CLA.

Do Google FTEs need to sign this CLA as well?

@egranata, yes, but no. It is already signed but you need to help the bot know you are a corporate user by logging in to it with your @google.com account.

@egranata, yes, but no. It is already signed but you need to help the bot know you are a corporate user by logging in to it with your @google.com account.

Aha, done. I was signed in, but I had never allowed the EasyCLA OAuth agent. Now it looks like it's green. Thanks for the heads up!

Needs language label to pass "Mergeable" check

Needs language label to pass "Mergeable" check

How do I add it? I see a Labels sidebar, but I don't think I can edit it

I've added the necessary labels and triggered the tests to run. It looks like there are a lot of failures, so please take a look at them.

I looked at some of the build failures, looks like I had a few compile errors that my local build didn't catch. I think I addressed them, but happy to get new results & keep iterating. Unfortunately, I don't have access to anything but a Linux host.

Please also update the PR description into a form that will be appropriate for the gRPC release notes.

What's a good PR to look at for this?

Reviewed 28 of 28 files at r1.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @egranata)

BUILD, line 753 at r1 (raw file):

        "src/core/lib/iomgr/lockfree_event.cc",
        "src/core/lib/iomgr/logical_thread.cc",
        "src/core/lib/iomgr/novsock.cc",

To match our existing naming convention, please call these two files resolve_vsock_linux.cc and resolve_vsock_fallback.cc, respectively.

Done

BUILD, line 1060 at r1 (raw file):

        "src/core/ext/filters/client_channel/local_subchannel_pool.cc",
        "src/core/ext/filters/client_channel/parse_address.cc",
        "src/core/ext/filters/client_channel/parse_address_novsock.cc",

Now that we're needing more conditional compilation here, we should probably bite the bullet and move the parse_address library from the client_channel tree into the iomgr tree, since we're not really supposed to have any ifdefs outside of that tree.

I can give it a shot, but I don't know if I am familiar enough with gRPC to do a good job at it.

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' &&

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.

Same as above

src/core/lib/iomgr/sockaddr_utils.cc, line 253 at r1 (raw file):

    case GRPC_AF_UNIX:
      return "unix";
#ifdef GRPC_AF_VSOCK

Why do we need an ifdef for vsock here but we don't need it for unix? It would be better to be consistent.

Because AF_UNIX as a symbol is pretty much defined anywhere (including on Windows, much to my surprise!) but AF_VSOCK is not. So we have to guard its usage. One thing I could do of course is

#ifdef HAVE_VSOCK
#define GRPC_AF_VSOCK AF_VSOCK
#else
#define GRPC_AF_VSOCK (-1)
#endif

just to have the symbol resolve and avoid the ifdef. Would that work better?

src/core/lib/iomgr/sockaddr_utils.cc, line 275 at r1 (raw file):

    case GRPC_AF_INET6:
      return grpc_ntohs(((grpc_sockaddr_in6*)addr)->sin6_port);
#ifdef GRPC_AF_VSOCK

As above, this ifdef should not be needed.

See above

src/core/lib/iomgr/sockaddr_utils.cc, line 277 at r1 (raw file):

#ifdef GRPC_AF_VSOCK
    case GRPC_AF_VSOCK:
#ifdef GRPC_HAVE_LINUX_VSOCK

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.

You'd still need the #ifdef GRPC_AF_VSOCK (but see above, or happy to take suggestions). Otherwise, I can make that change yes.

src/core/lib/iomgr/sockaddr_utils.cc, line 310 at r1 (raw file):

          grpc_htons(static_cast<uint16_t>(port));
      return 1;
#ifdef GRPC_AF_VSOCK

This ifdef should not be needed.

src/core/lib/iomgr/sockaddr_utils.cc, line 312 at r1 (raw file):

#ifdef GRPC_AF_VSOCK
    case GRPC_AF_VSOCK:
#ifdef GRPC_HAVE_LINUX_VSOCK

Same comment as on line 277 above.

See above. It seems like a lot of the feedback is about how to avoid the #ifdef GRPC_AF_VSOCK sprinkled around. I am happy to figure out a good path for that and go with it. What do you think?

Friendly ping

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!

Ah, @markdroth thanks for getting back to me; I will attempt to update the pull request soon & get back to you w/ an update

Apologies for the delay as well; this did fall off my radar w/ WFH & all