From 43f2d5a5d2760ca93e62bb8fb5ab0d02bbd6fc20 Mon Sep 17 00:00:00 2001
From: Tiago Castro <tiagolobocastro@gmail.com>
Date: Fri, 25 Aug 2023 17:11:41 +0100
Subject: [PATCH] fix(pcie): spdk requires nvme path id

When destroying a pcie nvme bdev we must not specify the nvme path id.
Also it was noticed that when destroy fails we end up with no alias which
could cause some panics. For this particular case we now add the alias
back if the bdev is still present though we need a more robust fix..

Signed-off-by: Tiago Castro <tiagolobocastro@gmail.com>
---
 io-engine/src/bdev/nvme.rs     | 18 ++++++++++++++++--
 io-engine/src/lvs/lvs_store.rs |  6 +++---
 test/grpc/test_nexus.js        |  4 ++--
 3 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/io-engine/src/bdev/nvme.rs b/io-engine/src/bdev/nvme.rs
index d0a08bf8b..891e824fc 100644
--- a/io-engine/src/bdev/nvme.rs
+++ b/io-engine/src/bdev/nvme.rs
@@ -11,7 +11,12 @@ use url::Url;
 
 use spdk_rs::{
     ffihelper::copy_str_with_null,
-    libspdk::{bdev_nvme_create, bdev_nvme_delete, spdk_nvme_transport_id},
+    libspdk::{
+        bdev_nvme_create,
+        bdev_nvme_delete,
+        nvme_path_id,
+        spdk_nvme_transport_id,
+    },
 };
 
 use crate::{
@@ -130,12 +135,21 @@ impl CreateDestroy for NVMe {
     async fn destroy(self: Box<Self>) -> Result<(), Self::Error> {
         if let Some(mut bdev) = UntypedBdev::lookup_by_name(&self.get_name()) {
             bdev.remove_alias(self.url.as_ref());
+
+            let mut path_id = nvme_path_id::default();
+            copy_str_with_null(&self.name, &mut path_id.trid.traddr);
+            path_id.trid.trtype = spdk_rs::libspdk::SPDK_NVME_TRANSPORT_PCIE;
+
             let errno = unsafe {
                 bdev_nvme_delete(
                     self.name.clone().into_cstring().as_ptr(),
-                    std::ptr::null(),
+                    &path_id,
                 )
             };
+            if errno != 0 {
+                UntypedBdev::lookup_by_name(&self.get_name())
+                    .map(|mut b| b.add_alias(self.url.as_ref()));
+            }
             errno_result_from_i32((), errno).context(
                 bdev_api::DestroyBdevFailed {
                     name: self.name.clone(),
diff --git a/io-engine/src/lvs/lvs_store.rs b/io-engine/src/lvs/lvs_store.rs
index e612dd6b1..416879a53 100644
--- a/io-engine/src/lvs/lvs_store.rs
+++ b/io-engine/src/lvs/lvs_store.rs
@@ -290,7 +290,7 @@ impl Lvs {
                 lvs.name()
             );
             let pool_name = lvs.name().to_string();
-            lvs.export().await.unwrap();
+            lvs.export().await?;
             Err(Error::Import {
                 source: Errno::EINVAL,
                 name: pool_name,
@@ -553,7 +553,7 @@ impl Lvs {
 
         info!("{}: lvs exported successfully", self_str);
 
-        bdev_destroy(&base_bdev.bdev_uri_original_str().unwrap())
+        bdev_destroy(&base_bdev.bdev_uri_original_str().unwrap_or_default())
             .await
             .map_err(|e| Error::Destroy {
                 source: e,
@@ -647,7 +647,7 @@ impl Lvs {
 
         info!("{}: lvs destroyed successfully", self_str);
 
-        bdev_destroy(&base_bdev.bdev_uri_original_str().unwrap())
+        bdev_destroy(&base_bdev.bdev_uri_original_str().unwrap_or_default())
             .await
             .map_err(|e| Error::Destroy {
                 source: e,
diff --git a/test/grpc/test_nexus.js b/test/grpc/test_nexus.js
index 7536db289..1cd7b5c00 100644
--- a/test/grpc/test_nexus.js
+++ b/test/grpc/test_nexus.js
@@ -779,7 +779,7 @@ describe('nexus', function () {
       };
       client.createNexusV2(args, (err) => {
         if (!err) return done(new Error('Expected error'));
-        assert.equal(err.code, grpc.status.INTERNAL);
+        assert.equal(err.code, grpc.status.INVALID_ARGUMENT);
         done();
       });
     });
@@ -799,7 +799,7 @@ describe('nexus', function () {
       };
       client.createNexusV2(args, (err) => {
         if (!err) return done(new Error('Expected error'));
-        assert.equal(err.code, grpc.status.INTERNAL);
+        assert.equal(err.code, grpc.status.INVALID_ARGUMENT);
         done();
       });
     });