support UNIX sockets #49

r-c-f · 2021-03-17T09:28:34Z

Use an extended getaddrinfo_unix which will treat any absolute path
(i.e. address starting with '/') as a place to bind a UNIX domain
socket. Store the socket path in the main nvnc structure to allow for
cleanup on closure. Closes #1.

any1 · 2021-04-02T22:10:29Z

src/server.c

+		return -1;
+	if (!(addr = malloc(sizeof(*addr))))
+		return -1;
+	if (!(*res = calloc(1, sizeof(**res)))) {


This makes assumptions about the inner workings of the C standard library.

any1 · 2021-04-02T22:13:28Z

src/server.c

@@ -1009,19 +1010,49 @@ static void on_connection(void* obj)
 	free(client);
 }

-static int bind_address(const char* name, int port)
+static int getaddrinfo_unix(const char *name, const char *service, struct addrinfo *hints, struct addrinfo **res)


In this project, the * is placed to the left.

any1 · 2021-04-02T22:16:04Z

src/server.c

+	return 0;
+}
+
+static int bind_address(const char* name, int port, bool unix_)


shoehorning unix sockets into this function is a bit messy. There should be two functions instead: bind_address_tcp and bind_address_unix.

any1 · 2021-04-02T22:21:30Z

src/server.c

@@ -1081,8 +1112,7 @@ static void on_main_dispatch(void* aml_obj)
 		process_fb_update_requests(client);
 }

-EXPORT
-struct nvnc* nvnc_open(const char* address, uint16_t port)
+static struct nvnc* open_common(const char *address, uint16_t port, bool unix_)


Let's call that boolean is_unix. This is OK, but I usually try to avoid boolean arguments like this because they make code harder to read. When you come across a line in code like this:

open_connection(address, 0, true)

you have to look at the definition of the function to see what it does. However, when you read something like

open_connection(address, 0, SOCKTYPE_UNIX);

you don't really have to look it up.

any1 · 2021-04-02T22:30:52Z

src/server.c

@@ -1094,7 +1124,8 @@ struct nvnc* nvnc_open(const char* address, uint16_t port)

 	LIST_INIT(&self->clients);

-	self->fd = bind_address(address, port);
+	self->fd = bind_address(address, port, unix_);
+


Let's not add an empty line here.

any1 · 2021-04-02T22:32:21Z

src/server.c

@@ -1125,9 +1156,25 @@ struct nvnc* nvnc_open(const char* address, uint16_t port)
 	aml_unref(self->poll_handle);
 failure:
 	close(self->fd);
+	if (unix_) {


What happens if someone tried to create a socket that already existed? It seems that this will cause the already existing socket to be unlinked.

any1 · 2021-04-02T22:34:16Z

src/server.c

@@ -1140,6 +1187,16 @@ void nvnc_close(struct nvnc* self)
 	LIST_FOREACH_SAFE (client, &self->clients, link, tmp)
 		client_unref(client);

+	struct sockaddr_storage addr;


I think that this code block can be extracted to a function to keep nvnc_close short and easy to read.

any1 · 2021-04-02T22:38:29Z

src/server.c

+	struct sockaddr_storage addr;
+	struct sockaddr_un *addr_unix;
+	socklen_t addr_len = sizeof(addr);
+	if (!getsockname(self->fd, (struct sockaddr*)&addr, &addr_len)) {


This project compares against zero to check for errors. I.e. we use == 0, not !.

any1 · 2021-04-03T18:27:07Z

src/server.c

@@ -1081,8 +1106,12 @@ static void on_main_dispatch(void* aml_obj)
 		process_fb_update_requests(client);
 }

-EXPORT
-struct nvnc* nvnc_open(const char* address, uint16_t port)
+enum addrtype {


definitions such as this one belong up top, between includes and function prototypes.

any1 · 2021-04-03T18:28:44Z

src/server.c

+		self->fd = bind_address_unix(address);
+		break;
+	default:
+		self->fd = -1;


This situation should never arise if the code is correctly written, so abort() will do nicely.

any1 · 2021-04-03T18:36:44Z

src/server.c

+
+static void unlink_fd_path(int fd)
+{
+	struct sockaddr_storage addr;


Is sockaddr_storage really needed here?

any1

This is pretty close be being ready now.

any1 · 2021-04-04T11:20:38Z

src/server.c

@@ -1124,10 +1163,41 @@ struct nvnc* nvnc_open(const char* address, uint16_t port)
 poll_start_failure:
 	aml_unref(self->poll_handle);
 failure:
-	close(self->fd);
+	if (self->fd >= 0) {


Let's move failure: below close(self->fd) and call it bind_failure: instead of having this if here. listen_failure and handle_failure will also need to be added in reverse order above close().

any1 · 2021-04-04T11:22:13Z

src/server.c

+		return -1;
+	strcpy(addr.sun_path, name);
+
+	int fd;


Nit: There's no need to pre-declare fd here.

any1 · 2021-04-04T11:24:24Z

src/server.c

+	};
+
+	if (strlen(name) >= sizeof(addr.sun_path))
+		return -1;


Can we set errno to ENAMETOOLONG here?

any1 · 2021-04-04T11:40:04Z

src/server.c

@@ -1094,7 +1123,17 @@ struct nvnc* nvnc_open(const char* address, uint16_t port)

 	LIST_INIT(&self->clients);

-	self->fd = bind_address(address, port);
+	switch (type) {


It would be neat to turn extract this to a function called bind_address.

any1

Looks good after that one last comment has been addressed.

Can you squash into a single commit and force push?

any1 · 2021-04-04T16:58:59Z

src/server.c

+
+	switch (type) {
+	case ADDRTYPE_TCP:
+		fd = bind_address_tcp(name, port);


We can return straight from the switch and move the default case outside the switch. Then, -Wswitch will give a warning when not all enum names are used.

Adds support for UNIX domain sockets with `nvnc_open_unix()` function. Closes any1#1.

Makes use of the functionality added in any1/neatvnc#49 to support UNIX domain sockets with a command line flag.

any1 · 2021-04-04T20:24:40Z

Thanks!

Makes use of the functionality added in any1/neatvnc#49 to support UNIX domain sockets with a command line flag.

any1 requested changes Apr 2, 2021

View reviewed changes

any1 requested changes Apr 3, 2021

View reviewed changes

any1 requested changes Apr 4, 2021

View reviewed changes

any1 reviewed Apr 4, 2021

View reviewed changes

Support UNIX sockets

e4fb5b6

Adds support for UNIX domain sockets with `nvnc_open_unix()` function. Closes any1#1.

r-c-f force-pushed the af_unix branch from dfebd87 to e4fb5b6 Compare April 4, 2021 17:16

r-c-f added a commit to r-c-f/wayvnc that referenced this pull request Apr 4, 2021

Support UNIX domain sockets

d732e48

Makes use of the functionality added in any1/neatvnc#49 to support UNIX domain sockets with a command line flag.

any1 merged commit b320723 into any1:master Apr 4, 2021

r-c-f mentioned this pull request Apr 4, 2021

Support UNIX domain sockets any1/wayvnc#95

Merged

r-c-f added a commit to r-c-f/wayvnc that referenced this pull request Apr 5, 2021

Support UNIX domain sockets

c62b2e7

Makes use of the functionality added in any1/neatvnc#49 to support UNIX domain sockets with a command line flag.

any1 pushed a commit to any1/wayvnc that referenced this pull request Apr 6, 2021

Support UNIX domain sockets

81192ac

Makes use of the functionality added in any1/neatvnc#49 to support UNIX domain sockets with a command line flag.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support UNIX sockets #49

support UNIX sockets #49

r-c-f commented Mar 17, 2021

any1 Apr 2, 2021

any1 Apr 2, 2021

any1 Apr 2, 2021

any1 Apr 2, 2021

any1 Apr 2, 2021

any1 Apr 2, 2021

any1 Apr 2, 2021

any1 Apr 2, 2021

any1 Apr 3, 2021

any1 Apr 3, 2021

any1 Apr 3, 2021

any1 left a comment

any1 Apr 4, 2021

any1 Apr 4, 2021

any1 Apr 4, 2021

any1 Apr 4, 2021

any1 left a comment

any1 Apr 4, 2021

any1 commented Apr 4, 2021

support UNIX sockets #49

support UNIX sockets #49

Conversation

r-c-f commented Mar 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

any1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

any1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

any1 commented Apr 4, 2021