[libvirt PATCH v2 08/10] nodedev: add mdev support to virNodeDeviceDestroy()

Jonathon Jongsma posted 10 patches 5 years, 8 months ago
There is a newer version of this series
[libvirt PATCH v2 08/10] nodedev: add mdev support to virNodeDeviceDestroy()
Posted by Jonathon Jongsma 5 years, 8 months ago
Add the ability to destroy mdev node devices via the mdevctl utility.

Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
---
 src/node_device/node_device_driver.c | 46 ++++++++++++++++++++++++++++
 src/node_device/node_device_driver.h |  3 ++
 2 files changed, 49 insertions(+)

diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c
index dbc7eb4d1e..c956bb55fc 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -790,6 +790,45 @@ nodeDeviceCreateXML(virConnectPtr conn,
 }
 
 
+virCommandPtr
+nodeDeviceGetMdevctlStopCommand(const char *uuid,
+                                bool persist)
+{
+    g_autofree char *mdevctl = virFindFileInPath(MDEVCTL);
+    const char *subcommand;
+
+    if (!mdevctl)
+        return NULL;
+
+    if (persist)
+        subcommand = "undefine";
+    else
+        subcommand = "stop";
+
+    virCommandPtr cmd = virCommandNewArgList(mdevctl,
+                                             subcommand,
+                                             "-u",
+                                             uuid,
+                                             NULL);
+
+    return cmd;
+}
+
+static int
+virMdevctlStop(virNodeDeviceDefPtr def)
+{
+    int status;
+    g_autoptr(virCommand) cmd = NULL;
+
+    cmd = nodeDeviceGetMdevctlStopCommand(def->caps->data.mdev.uuid, false);
+
+    if (virCommandRun(cmd, &status) < 0 || status != 0)
+        return -1;
+
+    return 0;
+}
+
+
 int
 nodeDeviceDestroy(virNodeDevicePtr device)
 {
@@ -836,6 +875,13 @@ nodeDeviceDestroy(virNodeDevicePtr device)
         if (virVHBAManageVport(parent_host, wwpn, wwnn, VPORT_DELETE) < 0)
             goto cleanup;
 
+        ret = 0;
+    } else if (nodeDeviceHasCapability(def, VIR_NODE_DEV_CAP_MDEV)) {
+        if (virMdevctlStop(def) < 0) {
+            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                           _("Unable to stop mediated device"));
+            goto cleanup;
+        }
         ret = 0;
     } else {
         virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h
index 576f75375f..794cb5dc1c 100644
--- a/src/node_device/node_device_driver.h
+++ b/src/node_device/node_device_driver.h
@@ -122,3 +122,6 @@ virCommandPtr
 nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def,
                                  bool persist,
                                  char **uuid_out);
+virCommandPtr
+nodeDeviceGetMdevctlStopCommand(const char *uuid,
+                                bool persist);
-- 
2.21.3

Re: [libvirt PATCH v2 08/10] nodedev: add mdev support to virNodeDeviceDestroy()
Posted by Erik Skultety 5 years, 8 months ago
On Tue, Jun 09, 2020 at 04:43:48PM -0500, Jonathon Jongsma wrote:
> Add the ability to destroy mdev node devices via the mdevctl utility.
>
> Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
> ---
>  src/node_device/node_device_driver.c | 46 ++++++++++++++++++++++++++++
>  src/node_device/node_device_driver.h |  3 ++
>  2 files changed, 49 insertions(+)
>
> diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c
> index dbc7eb4d1e..c956bb55fc 100644
> --- a/src/node_device/node_device_driver.c
> +++ b/src/node_device/node_device_driver.c
> @@ -790,6 +790,45 @@ nodeDeviceCreateXML(virConnectPtr conn,
>  }
>
>
> +virCommandPtr
> +nodeDeviceGetMdevctlStopCommand(const char *uuid,
> +                                bool persist)
> +{
> +    g_autofree char *mdevctl = virFindFileInPath(MDEVCTL);
> +    const char *subcommand;
> +
> +    if (!mdevctl)
> +        return NULL;
> +
> +    if (persist)
> +        subcommand = "undefine";

"undefine" is a NOP on active mdevs IIRC, so again the helper name is
confusing.

> +    else
> +        subcommand = "stop";
> +
> +    virCommandPtr cmd = virCommandNewArgList(mdevctl,
> +                                             subcommand,
> +                                             "-u",
> +                                             uuid,
> +                                             NULL);
> +
> +    return cmd;
> +}

Like I mentioned already, we could have a generic translator to the mdevctl
subcommands.

Regards,
Erik

Re: [libvirt PATCH v2 08/10] nodedev: add mdev support to virNodeDeviceDestroy()
Posted by Jonathon Jongsma 5 years, 8 months ago
On Thu, 2020-06-11 at 16:00 +0200, Erik Skultety wrote:
> On Tue, Jun 09, 2020 at 04:43:48PM -0500, Jonathon Jongsma wrote:
> > Add the ability to destroy mdev node devices via the mdevctl
> > utility.
> > 
> > Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
> > ---
> >  src/node_device/node_device_driver.c | 46
> > ++++++++++++++++++++++++++++
> >  src/node_device/node_device_driver.h |  3 ++
> >  2 files changed, 49 insertions(+)
> > 
> > diff --git a/src/node_device/node_device_driver.c
> > b/src/node_device/node_device_driver.c
> > index dbc7eb4d1e..c956bb55fc 100644
> > --- a/src/node_device/node_device_driver.c
> > +++ b/src/node_device/node_device_driver.c
> > @@ -790,6 +790,45 @@ nodeDeviceCreateXML(virConnectPtr conn,
> >  }
> > 
> > 
> > +virCommandPtr
> > +nodeDeviceGetMdevctlStopCommand(const char *uuid,
> > +                                bool persist)
> > +{
> > +    g_autofree char *mdevctl = virFindFileInPath(MDEVCTL);
> > +    const char *subcommand;
> > +
> > +    if (!mdevctl)
> > +        return NULL;
> > +
> > +    if (persist)
> > +        subcommand = "undefine";
> 
> "undefine" is a NOP on active mdevs IIRC, so again the helper name is
> confusing.

Oh, you're right. This part was meant to plan ahead for persistent
mediated devices, but since it's not yet used (and since it doesn't
have the effect intended, as you point out), it should probably just be
removed from this patch series.


> 
> > +    else
> > +        subcommand = "stop";
> > +
> > +    virCommandPtr cmd = virCommandNewArgList(mdevctl,
> > +                                             subcommand,
> > +                                             "-u",
> > +                                             uuid,
> > +                                             NULL);
> > +
> > +    return cmd;
> > +}
> 
> Like I mentioned already, we could have a generic translator to the
> mdevctl
> subcommands.
> 
> Regards,
> Erik

Re: [libvirt PATCH v2 08/10] nodedev: add mdev support to virNodeDeviceDestroy()
Posted by Michal Privoznik 5 years, 8 months ago
On 6/9/20 11:43 PM, Jonathon Jongsma wrote:
> Add the ability to destroy mdev node devices via the mdevctl utility.
> 
> Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
> ---
>   src/node_device/node_device_driver.c | 46 ++++++++++++++++++++++++++++
>   src/node_device/node_device_driver.h |  3 ++
>   2 files changed, 49 insertions(+)
> 
> diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c
> index dbc7eb4d1e..c956bb55fc 100644
> --- a/src/node_device/node_device_driver.c
> +++ b/src/node_device/node_device_driver.c
> @@ -790,6 +790,45 @@ nodeDeviceCreateXML(virConnectPtr conn,
>   }
>   
>   
> +virCommandPtr
> +nodeDeviceGetMdevctlStopCommand(const char *uuid,
> +                                bool persist)
> +{
> +    g_autofree char *mdevctl = virFindFileInPath(MDEVCTL);

Same comment about virFindFileInPath() as in one of previous patches.

> +    const char *subcommand;
> +
> +    if (!mdevctl)
> +        return NULL;
> +
> +    if (persist)
> +        subcommand = "undefine";
> +    else
> +        subcommand = "stop";
> +
> +    virCommandPtr cmd = virCommandNewArgList(mdevctl,
> +                                             subcommand,
> +                                             "-u",
> +                                             uuid,
> +                                             NULL);

We don't really like variables being defined in the middle of a block. 
Fortunately, the variable is not really needed and this can be turned 
into "return virCommandNewArgList(...)"

Squash this in:

diff --git i/src/node_device/node_device_driver.c 
w/src/node_device/node_device_driver.c
index 23d18308f7..2c204c7a83 100644
--- i/src/node_device/node_device_driver.c
+++ w/src/node_device/node_device_driver.c
@@ -790,26 +790,21 @@ virCommandPtr
  nodeDeviceGetMdevctlStopCommand(const char *uuid,
                                  bool persist)
  {
-    g_autofree char *mdevctl = virFindFileInPath(MDEVCTL);
      const char *subcommand;

-    if (!mdevctl)
-        return NULL;
-
      if (persist)
          subcommand = "undefine";
      else
          subcommand = "stop";

-    virCommandPtr cmd = virCommandNewArgList(mdevctl,
-                                             subcommand,
-                                             "-u",
-                                             uuid,
-                                             NULL);
-
-    return cmd;
+    return virCommandNewArgList(MDEVCTL,
+                                subcommand,
+                                "-u",
+                                uuid,
+                                NULL);
  }

+
  static int
  virMdevctlStop(virNodeDeviceDefPtr def)
  {



Michal