vhost_scsi: vhost_scsi_dev_add_dev now takes vhost_dev param

Continuation of patch 94afad5a [1].
That function could be called from outside of the vhost reactor,
causing data races and possible segfaults.

Now, after this patch, all ctrlr-changing functions take spdk_vhost_dev
parameter, meaning they should be called via external event API [1],
which soon will be the only way of obtaining spdk_vhost_dev pointer.

[1] 94afad5a ("vhost: added external API to call spdk_events on vdev reactor")

Change-Id: I40ea66ad09fb5c433dd897a4e22aedeb423f9b4b
Signed-off-by: Dariusz Stojaczyk <dariuszx.stojaczyk@intel.com>
Reviewed-on: https://review.gerrithub.io/371013
Tested-by: SPDK Automated Test System <sys_sgsw@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
Reviewed-by: Daniel Verkamp <daniel.verkamp@intel.com>
Reviewed-by: Changpeng Liu <changpeng.liu@intel.com>

include/spdk/vhost.h

@@ -71,7 +71,8 @@ int spdk_vhost_scsi_controller_construct(void);
 int spdk_vhost_scsi_dev_construct(const char *name, const char *cpumask);
 int spdk_vhost_scsi_dev_remove(struct spdk_vhost_dev *vdev);
 struct spdk_scsi_dev *spdk_vhost_scsi_dev_get_dev(struct spdk_vhost_dev *ctrl, uint8_t num);
-int spdk_vhost_scsi_dev_add_dev(const char *name, unsigned scsi_dev_num, const char *lun_name);
+int spdk_vhost_scsi_dev_add_dev(struct spdk_vhost_dev *vdev, unsigned scsi_dev_num,
+				const char *lun_name);
 int spdk_vhost_scsi_dev_remove_dev(struct spdk_vhost_dev *vdev, unsigned scsi_dev_num);
 
 int spdk_vhost_blk_construct(const char *name, const char *cpumask, const char *dev_name,

lib/vhost/vhost_rpc.c

@@ -186,6 +186,7 @@ spdk_rpc_add_vhost_scsi_lun(struct spdk_jsonrpc_request *request,
 {
 	struct rpc_add_vhost_scsi_ctrlr_lun req = {0};
 	struct spdk_json_write_ctx *w;
+	struct spdk_vhost_dev *vdev;
 	int rc;
 	char buf[64];
 
@@ -197,7 +198,14 @@ spdk_rpc_add_vhost_scsi_lun(struct spdk_jsonrpc_request *request,
 		goto invalid;
 	}
 
-	rc = spdk_vhost_scsi_dev_add_dev(req.ctrlr, req.scsi_dev_num, req.lun_name);
+	vdev = spdk_vhost_dev_find(req.ctrlr);
+	if (vdev == NULL) {
+		SPDK_ERRLOG("Controller %s is not defined.\n", req.ctrlr);
+		rc = -ENODEV;
+		goto invalid;
+	}
+
+	rc = spdk_vhost_scsi_dev_add_dev(vdev, req.scsi_dev_num, req.lun_name);
 	if (rc < 0) {
 		goto invalid;
 	}

lib/vhost/vhost_scsi.c

@@ -801,17 +801,17 @@ spdk_vhost_scsi_lun_hotremove(const struct spdk_scsi_lun *lun, void *arg)
 }
 
 int
-spdk_vhost_scsi_dev_add_dev(const char *ctrlr_name, unsigned scsi_dev_num, const char *lun_name)
+spdk_vhost_scsi_dev_add_dev(struct spdk_vhost_dev *vdev, unsigned scsi_dev_num,
+			    const char *lun_name)
 {
 	struct spdk_vhost_scsi_dev *svdev;
-	struct spdk_vhost_dev *vdev;
 	struct spdk_scsi_dev *scsi_dev;
 	char dev_name[SPDK_SCSI_DEV_MAX_NAME];
 	int lun_id_list[1];
 	char *lun_names_list[1];
 
-	if (ctrlr_name == NULL) {
+	svdev = to_scsi_dev(vdev);
-		SPDK_ERRLOG("No controller name\n");
+	if (svdev == NULL) {
 		return -EINVAL;
 	}
 
@@ -829,12 +829,6 @@ spdk_vhost_scsi_dev_add_dev(const char *ctrlr_name, unsigned scsi_dev_num, const
 		return -1;
 	}
 
-	vdev = spdk_vhost_dev_find(ctrlr_name);
-	if (vdev == NULL) {
-		SPDK_ERRLOG("Controller %s is not defined.\n", ctrlr_name);
-		return -ENODEV;
-	}
-
 	svdev = to_scsi_dev(vdev);
 	if (svdev == NULL) {
 		return -EINVAL;
@@ -842,12 +836,12 @@ spdk_vhost_scsi_dev_add_dev(const char *ctrlr_name, unsigned scsi_dev_num, const
 
 	if (vdev->lcore != -1 && !spdk_vhost_dev_has_feature(vdev, VIRTIO_SCSI_F_HOTPLUG)) {
 		SPDK_ERRLOG("%s: 'Dev %u' is in use and hot-attach is not enabled for this controller\n",
-			    ctrlr_name, scsi_dev_num);
+			    vdev->name, scsi_dev_num);
 		return -ENOTSUP;
 	}
 
 	if (svdev->scsi_dev[scsi_dev_num] != NULL) {
-		SPDK_ERRLOG("Controller %s dev %u already occupied\n", ctrlr_name, scsi_dev_num);
+		SPDK_ERRLOG("Controller %s dev %u already occupied\n", vdev->name, scsi_dev_num);
 		return -EEXIST;
 	}
 
@@ -926,6 +920,7 @@ int
 spdk_vhost_scsi_controller_construct(void)
 {
 	struct spdk_conf_section *sp = spdk_conf_first_section(NULL);
+	struct spdk_vhost_dev *vdev;
 	int i, dev_num;
 	unsigned ctrlr_num = 0;
 	char *lun_name, *dev_num_str;
@@ -951,6 +946,9 @@ spdk_vhost_scsi_controller_construct(void)
 			return -1;
 		}
 
+		vdev = spdk_vhost_dev_find(name);
+		assert(vdev);
+
 		for (i = 0; spdk_conf_section_get_nval(sp, "Dev", i) != NULL; i++) {
 			dev_num_str = spdk_conf_section_get_nmval(sp, "Dev", i, 0);
 			if (dev_num_str == NULL) {
@@ -968,7 +966,7 @@ spdk_vhost_scsi_controller_construct(void)
 				return -1;
 			}
 
-			if (spdk_vhost_scsi_dev_add_dev(name, dev_num, lun_name) < 0) {
+			if (spdk_vhost_scsi_dev_add_dev(vdev, dev_num, lun_name) < 0) {
 				return -1;
 			}
 		}

test/unit/lib/vhost/vhost_scsi.c/vhost_scsi_ut.c

@@ -305,52 +305,49 @@ vhost_scsi_dev_add_dev_test(void)
 	int rc;
 	char long_name[SPDK_SCSI_DEV_MAX_NAME + 1];
 	struct spdk_vhost_scsi_dev *svdev;
+	struct spdk_vhost_dev *vdev;
 	struct spdk_scsi_dev *scsi_dev;
 
 	/* Add device to controller without name */
 	rc = spdk_vhost_scsi_dev_add_dev(NULL, 0, "Malloc0");
 	CU_ASSERT(rc == -EINVAL);
 
+	svdev = alloc_svdev();
+	vdev = &svdev->vdev;
+	MOCK_SET(spdk_vhost_dev_has_feature, bool, false);
+
 	/* Add device when max devices is reached */
-	rc = spdk_vhost_scsi_dev_add_dev("vhost.0",
+	rc = spdk_vhost_scsi_dev_add_dev(vdev,
 					 SPDK_VHOST_SCSI_CTRLR_MAX_DEVS + 1, "Malloc0");
 	CU_ASSERT(rc == -EINVAL);
 
 	/* Add device but lun has no name */
-	rc = spdk_vhost_scsi_dev_add_dev("vhost.0", 0, NULL);
+	rc = spdk_vhost_scsi_dev_add_dev(vdev, 0, NULL);
 	CU_ASSERT(rc == -EINVAL);
 
 	/* Add device but lun has too long name */
 	memset(long_name, 'x', sizeof(long_name));
 	long_name[SPDK_SCSI_DEV_MAX_NAME] = 0;
-	rc = spdk_vhost_scsi_dev_add_dev("vhost.0", 0, long_name);
+	rc = spdk_vhost_scsi_dev_add_dev(vdev, 0, long_name);
 	CU_ASSERT(rc != 0);
 
-	/* Add device to not defined controller */
-	MOCK_SET_P(spdk_vhost_dev_find, struct spdk_vhost_dev *, NULL);
-	rc = spdk_vhost_scsi_dev_add_dev("vhost.0", 0, "Malloc0");
-	CU_ASSERT(rc == -ENODEV);
-
 	/* Add device to a controller which is in use */
-	svdev = alloc_svdev();
 	svdev->vdev.lcore = 0;
-	MOCK_SET_P(spdk_vhost_dev_find, struct spdk_vhost_dev *, &svdev->vdev);
+	rc = spdk_vhost_scsi_dev_add_dev(vdev, 0, "Malloc0");
-	MOCK_SET(spdk_vhost_dev_has_feature, bool, false);
-	rc = spdk_vhost_scsi_dev_add_dev("vhost.0", 0, "Malloc0");
 	CU_ASSERT(rc == -ENOTSUP);
 
 	/* Add device to controller with already occupied device */
-	svdev->vdev.lcore = -1;
+	vdev->lcore = -1;
 	scsi_dev = alloc_scsi_dev();
 	svdev->scsi_dev[0] = scsi_dev;
-	rc = spdk_vhost_scsi_dev_add_dev("vhost.0", 0, "Malloc0");
+	rc = spdk_vhost_scsi_dev_add_dev(vdev, 0, "Malloc0");
 	CU_ASSERT(rc == -EEXIST);
 	free(scsi_dev);
 	svdev->scsi_dev[0] = NULL;
 
 	/* Failed to create device */
 	MOCK_SET_P(spdk_scsi_dev_construct, struct spdk_scsi_dev *, NULL);
-	rc = spdk_vhost_scsi_dev_add_dev("vhost.0", 0, "Malloc0");
+	rc = spdk_vhost_scsi_dev_add_dev(vdev, 0, "Malloc0");
 	CU_ASSERT(rc == -EINVAL);
 
 	free(svdev);