diff mbox series

[v3,2/5] vhost: make use of FD manager init function

Message ID 20240409114845.1336403-3-maxime.coquelin@redhat.com (mailing list archive)
State New
Delegated to: Maxime Coquelin
Headers
Series vhost: FD manager improvements |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Maxime Coquelin April 9, 2024, 11:48 a.m. UTC 
  Instead of statically initialize the fdset, this patch
converts VDUSE and Vhost-user to use fdset_init() function,
which now also initialize the mutexes.

This is preliminary rework to hide FDs manager pipe from
its users.

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/vhost/fd_man.c | 11 +++++++++--
 lib/vhost/fd_man.h |  2 +-
 lib/vhost/socket.c | 12 +++++-------
 lib/vhost/vduse.c  | 15 ++++++---------
 4 files changed, 21 insertions(+), 19 deletions(-)

Comments

Stephen Hemminger April 9, 2024, 4:38 p.m. UTC | #1 
On Tue,  9 Apr 2024 13:48:42 +0200
Maxime Coquelin <maxime.coquelin@redhat.com> wrote:

> -void
> +int
>  fdset_init(struct fdset *pfdset)
>  {
>  	int i;
>  
>  	if (pfdset == NULL)
> -		return;
> +		return -1;

This test is unnecessary. The function is not exported, and therefore
can only be called from vhost library. And all call sites are passing
a pointer to local data.

Adding extra parameter checks to internal functions bloats code
and creates lots of untestable paths. Coverity might even be
smart enough to detect these.
Maxime Coquelin April 10, 2024, 6:22 a.m. UTC | #2 
On 4/9/24 18:38, Stephen Hemminger wrote:
> On Tue,  9 Apr 2024 13:48:42 +0200
> Maxime Coquelin <maxime.coquelin@redhat.com> wrote:
> 
>> -void
>> +int
>>   fdset_init(struct fdset *pfdset)
>>   {
>>   	int i;
>>   
>>   	if (pfdset == NULL)
>> -		return;
>> +		return -1;
> 
> This test is unnecessary. The function is not exported, and therefore
> can only be called from vhost library. And all call sites are passing
> a pointer to local data.
> 
> Adding extra parameter checks to internal functions bloats code
> and creates lots of untestable paths. Coverity might even be
> smart enough to detect these.
> 

Agree. I will remove the test in next revision.

Thanks,
Maxime
diff mbox series

Patch

diff --git a/lib/vhost/fd_man.c b/lib/vhost/fd_man.c
index 67ee1589e1..e42c491fdb 100644
--- a/lib/vhost/fd_man.c
+++ b/lib/vhost/fd_man.c
@@ -96,19 +96,26 @@  fdset_add_fd(struct fdset *pfdset, int idx, int fd,
 	pfd->revents = 0;
 }
 
-void
+int
 fdset_init(struct fdset *pfdset)
 {
 	int i;
 
 	if (pfdset == NULL)
-		return;
+		return -1;
+
+	pthread_mutex_init(&pfdset->fd_mutex, NULL);
+	pthread_mutex_init(&pfdset->fd_polling_mutex, NULL);
+	pthread_mutex_init(&pfdset->sync_mutex, NULL);
+	pthread_cond_init(&pfdset->sync_cond, NULL);
 
 	for (i = 0; i < MAX_FDS; i++) {
 		pfdset->fd[i].fd = -1;
 		pfdset->fd[i].dat = NULL;
 	}
 	pfdset->num = 0;
+
+	return 0;
 }
 
 /**
diff --git a/lib/vhost/fd_man.h b/lib/vhost/fd_man.h
index 4e00f94758..0f4cddfe56 100644
--- a/lib/vhost/fd_man.h
+++ b/lib/vhost/fd_man.h
@@ -43,7 +43,7 @@  struct fdset {
 };
 
 
-void fdset_init(struct fdset *pfdset);
+int fdset_init(struct fdset *pfdset);
 
 int fdset_add(struct fdset *pfdset, int fd,
 	fd_cb rcb, fd_cb wcb, void *dat);
diff --git a/lib/vhost/socket.c b/lib/vhost/socket.c
index 96b3ab5595..8155749972 100644
--- a/lib/vhost/socket.c
+++ b/lib/vhost/socket.c
@@ -89,13 +89,6 @@  static int create_unix_socket(struct vhost_user_socket *vsocket);
 static int vhost_user_start_client(struct vhost_user_socket *vsocket);
 
 static struct vhost_user vhost_user = {
-	.fdset = {
-		.fd = { [0 ... MAX_FDS - 1] = {-1, NULL, NULL, NULL, 0} },
-		.fd_mutex = PTHREAD_MUTEX_INITIALIZER,
-		.fd_pooling_mutex = PTHREAD_MUTEX_INITIALIZER,
-		.sync_mutex = PTHREAD_MUTEX_INITIALIZER,
-		.num = 0
-	},
 	.vsocket_cnt = 0,
 	.mutex = PTHREAD_MUTEX_INITIALIZER,
 };
@@ -1188,6 +1181,11 @@  rte_vhost_driver_start(const char *path)
 		return vduse_device_create(path, vsocket->net_compliant_ol_flags);
 
 	if (fdset_tid.opaque_id == 0) {
+		if (fdset_init(&vhost_user.fdset) < 0) {
+			VHOST_CONFIG_LOG(path, ERR, "failed to init Vhost-user fdset");
+			return -1;
+		}
+
 		/**
 		 * create a pipe which will be waited by poll and notified to
 		 * rebuild the wait list of poll.
diff --git a/lib/vhost/vduse.c b/lib/vhost/vduse.c
index e0c6991b69..530c148399 100644
--- a/lib/vhost/vduse.c
+++ b/lib/vhost/vduse.c
@@ -31,15 +31,7 @@  struct vduse {
 	struct fdset fdset;
 };
 
-static struct vduse vduse = {
-	.fdset = {
-		.fd = { [0 ... MAX_FDS - 1] = {-1, NULL, NULL, NULL, 0} },
-		.fd_mutex = PTHREAD_MUTEX_INITIALIZER,
-		.fd_pooling_mutex = PTHREAD_MUTEX_INITIALIZER,
-		.sync_mutex = PTHREAD_MUTEX_INITIALIZER,
-		.num = 0
-	},
-};
+static struct vduse vduse;
 
 static bool vduse_events_thread;
 
@@ -435,6 +427,11 @@  vduse_device_create(const char *path, bool compliant_ol_flags)
 
 	/* If first device, create events dispatcher thread */
 	if (vduse_events_thread == false) {
+		if (fdset_init(&vduse.fdset) < 0) {
+			VHOST_CONFIG_LOG(path, ERR, "failed to init VDUSE fdset");
+			return -1;
+		}
+
 		/**
 		 * create a pipe which will be waited by poll and notified to
 		 * rebuild the wait list of poll.