On Thu, Aug 11, 2016 at 11:33:59AM -0400, Aaron Conole wrote:
> Ben Pfaff <[email protected]> writes:
>
> > On Mon, Aug 01, 2016 at 08:24:40AM -0400, Aaron Conole wrote:
> >> Aaron Conole <[email protected]> writes:
> >>
> >> > Currently, when using Open vSwitch with DPDK and qemu guests, the
> >> > recommended
> >> > method for joining the guests is via the dpdkvhostuser interface. This
> >> > interface uses Unix Domain sockets to communicate. When these sockets are
> >> > created, they inherit the permissions and ownership from the
> >> > vswitchd process.
> >> > This can lead to an undesirable state where the QEMU process cannot use
> >> > the
> >> > socket file until manual intervention is performed (via `chown`
> >> > and/or `chmod`
> >> > calls).
> >> >
> >> > This patchset gives the ability to set the permissions and ownership of
> >> > all
> >> > dpdkvhostuser sockets from the database, avoiding the manual intervention
> >> > required to connect QEMU and OVS via DPDK.
> >> >
> >> > The first patch adds chmod and chown calls to lib, with unit tests. The
> >> > second patch hooks those calls into the netdev_dpdk_vhost_user_construct
> >> > function, after the socket is created.
> >> >
> >> > Changes from v2:
> >> > * Added a new 2nd patch to series for chmod/chown on already opened
> >> > files.
> >> > There exist known implementations for other systems, including
> >> > FreeBSD, but
> >> > only linux is implemented. ENOTSUP is set when these calls fail
> >> > on non-linux
> >> > systems.
> >>
> >> Ping. I was hoping to have this applied before the 2.6 branch was cut;
> >> is that possible?
> >
> > The code in patch 2, which goes through every fd in /proc to find the
> > socket it needs because DPDK will not tell it the fd and will not set
> > the needed permissions, is appalling. It may be the biggest kluge I've
> > seen this year.
>
> Ouch. I'm sure that's an accomplishment I don't want associated with my
> name. Do you take issue with the design (I agree it's awful to need
> anything like this)? Did the implementation really smell that bad?
It's not about the code (and it's certainly not ad hominem). It's about
the whole technique. The idea of opening a socket, then scanning the
whole fd table by reading through the file system to determine the
number of the fd that was opened, is appalling.
> > This would be slightly less appalling if it used fstat() and
> > getsockname() instead of /proc. Still not great.
>
> I disagree - patch 2/3 wouldn't exist. [...]
The approach I was talking about was to scan the fd table using a loop
over the valid fds, calling fstat() on each to find out whether it's a
socket then getsockname() to find out whether it's the right socket.
Come to think of it, I don't know what happens if you try to
getsockname() on a socket whose name is longer than whatever fits in
sockaddr_un. Dammit.
> [...] That goes to the last sentence
> in this. Both of those calls will work for a descriptor (and notice
> that patch 1/3 uses them). The issue I have is trying to go from a
> pathname to a descriptor (which is not a standardized interface for a
> plethora of reasons). If I had the descriptor, though, patch 2/3 would
> never have been submitted (because it would be unneeded - I do
> fchmod/fchown in patch 1/3).
Yes, right. I understand the problem, I just don't understand why it
exists. The DPDK maintainers can solve it instantly in 5 minutes of
work.
> > This definitely needs comments to explain why it exists and what it's
> > doing.
Thanks for the explanation. In this case, I meant that the *code* needs
to have comments, since it had none. It took quite a while from looking
at the code to figure out what was going on, and longer to figure out
why. Comments should make this obvious to the reader.
[...]
> > This could be properly fixed in DPDK by adding a 1-line helper function
> > that returns the fd for a vhost-user server.
>
> +1
> I agree strongly. However, Christian Ehrhardt and myself have each
> submitted patches[5][6] to try and make this kind of thing non-odius.
> These patches have not been accepted.
Neither of those is a simple "return server->listen_fd;" function. Any
chance that would be accepted? It would solve the problem just fine.
> If you think that patch 2/3 is that maloderous that it cannot be
> accepted, then there will always probably be this kind of disjointed
> file-system permissions gap for vhost-user server mode interfaces.
> Unless you have a better suggestion - I'm all ears then :)
I do have one suggestion. Something like this:
for (int i = 0; i < 10; i++) {
/* Find the first available file descriptor, then close it. */
int fd = socket(...);
close(fd);
/* Create server socket. */
server = dpdk_create_server_socket(filename, ...);
/* Check that fd is the new server socket fd.
* This is normally true unless some other thread created a fd
* concurrently. */
struct stat s;
struct sockaddr_un sun;
socklen_t sun_len = sizeof sun;
if (!stat(fd, &s) && S_ISSOCK(s.st_mode) &&
getsockname(fd, &sun, &sun_len) && !strcmp(filename, sun.sun_path)) {
/* Yay, we found the fd. */
return success;
}
/* Race. Try again. */
dpdk_close_server_socket(server);
}
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
