This patch series follows the previously-merged series which added support for
transient mediated devices. This series expands mdev support to include
persistent device definitions. Again, it relies on mdevctl as the backend.

It follows the common libvirt pattern of APIs by adding the following new APIs
for node devices:
    - virNodeDeviceDefineXML() - defines a persistent device
    - virNodeDeviceUndefine() - undefines a persistent device
    - virNodeDeviceCreate() - starts a previously-defined device

It also adds virsh commands mapping to these new APIs: nodedev-define,
nodedev-undefine, and nodedev-start.

Since we rely on mdevctl for the definition of mediated devices, we need a way
to stay up-to-date with devices that are defined by mdevctl (outside of
libvirt).  The method for staying up-to-date is currently a little bit crude
due to the fact that mdevctl does not emit any events when new devices are
added or removed. As a workaround, we create a file monitor for the mdevctl
config directory and re-query mdevctl when we detect changes within that
directory. In the future, mdevctl may introduce a more elegant solution.

Changes in v6:
 - rebase to git master again
 - remove typedefs for various *Ptr types, since they're now discouraged in
   libvirt.

Changes in v5:
 - Rebase to git master
 - updated new API version info to 7.2.0
 - capture and relay stderr message from mdevctl
 - changed to using GHashTable functions directly instead of deprecated
   virHash functions
 - protected mdevctlMonitors with a mutex
 - added a couple patches to fix the 5s delay when defining a new mdev. These
   are currently separate patches added to the end of the series, but could be
   squashed into the earlier commits if that's preferred.
 - various other minor review fixes

Changes in v4:
 - rebase to git master
 - switch to throwaway thread for querying mdevctl
 - fixed a bug when removing devices because I was accidentally using
   virHashForEach() instead of virHashForeachSafe()
 - use DEFINED/UNDEFINED events instead of STARTED/STOPPED events
 - changes related to merging information about mdev devices from both udev a=
nd
   mdevctl:
   - Re-used the same function to copy extra data from mdevctl regardless of
     whether we're processing a udev event or a mdevctl event (recommended by
     Erik). This results in slightly more complex handling of the object
     lifetimes (see patch 9), but it consolidates some code.
   - nodeDeviceDefCopyFromMdevctl() previously only copied the data that was
     unique to mdevctl and didn't exist in udev. It now copies additional data
     (possibly overwriting some udev).  This solves a problem where a device =
is
     defined but not active (i.e.  we have not gotten any data from udev), and
     then changed (e.g. somebody calls 'mdevctl modify' to change the mdev
     type), but libvirt was not updating to the new definition.
 - fix a bug where we were mistakenly emitting 'update' events for devices th=
at
   had not changed
 - Added the ability to specify a uuid for an mdev via device XML.
 - split some commits into multiple patches
 - updated new API version info to 7.1.0
 - Fixed a bug reported by Yan Fu which hangs the client when attempting to
   destroy a nodedev that is in use by an active vm. See
   https://www.redhat.com/archives/libvir-list/2021-February/msg00116.html for
   solution suggested by Alex.
 - numerous smaller fixes from review findings

changes in v3:
 - streamlined tests -- removed some unnecessary duplication
 - split out patch to factor out node device name generation function
 - split nodeDeviceParseMdevctlChildDevice() into a separate function
 - added follow-up patch to remove space-padded alignment in header
 - refactored the mdevctl update handling significantly:
   - no longer a separate persistent thread that gets signaled by a timer
   - now piggybacks onto the existing udev thread and signals the thread in t=
he
     same way that the udev event does.
   - Daniel suggested spawning a throw-away thread to handle mdevctl updates,
     but that introduces the complexity of possibly serializing multiple
     throw-away threads (e.g. if we get an 'created' event followed immediate=
ly
     by a 'deleted' event, two threads may be spawned and we'd need to ensure
     they are properly ordered)
 - added virNodeDeviceObjListForEach() and virNodeDeviceObjListRemoveLocked()
   to simplify removing devices that are removed from mdevctl.
 - coding style fixes
 - NOTE: per Erik's request, I experimented with changing the way that mdevctl
   commands were generated and tested (e.g. introducing something like
   virMdevctlGetCommand(def, MDEVCTL_COMMAND_<SUBCOMMAND>, ...)), but it was
   too invasive and awkward and didn't seem worthwhile

Changes in v2:
 - rebase to latest git master

Jonathon Jongsma (30):
  nodedev: capture and report stderror from mdevctl
  tests: remove extra trailing semicolon
  nodedev: introduce concept of 'active' node devices
  nodedev: Add ability to filter by active state
  nodedev: fix docs for virConnectListAllNodeDevices()
  nodedev: expose internal helper for naming devices
  nodedev: add ability to parse mdevs from mdevctl
  nodedev: add ability to list defined mdevs
  nodedev: add persistence to virNodeDeviceObj
  nodedev: add DEFINED/UNDEFINED lifecycle events
  nodedev: add mdevctl devices to node device list
  nodedev: add helper functions to remove node devices
  nodedev: handle mdevs that disappear from mdevctl
  nodedev: Refresh mdev devices when changes are detected
  nodedev: add function to generate mdevctl define command
  api: add virNodeDeviceDefineXML()
  virsh: Add --inactive, --all to nodedev-list
  virsh: add nodedev-define command
  nodedev: refactor tests to support mdev undefine
  api: add virNodeDeviceUndefine()
  virsh: Factor out function to find node device
  virsh: add nodedev-undefine command
  api: add virNodeDeviceCreate()
  virsh: add "nodedev-start" command
  nodedev: add <uuid> element to mdev caps
  nodedev: add ability to specify UUID for new mdevs
  nodedev: fix hang when destroying an mdev in use
  nodedev: add docs about mdev attribute order
  nodedev: factor out function to add mediated devices
  nodedev: avoid delay when defining a new mdev

 docs/formatnode.html.in                       |   5 +-
 docs/schemas/nodedev.rng                      |  43 +-
 examples/c/misc/event-test.c                  |   4 +
 include/libvirt/libvirt-nodedev.h             |  20 +-
 src/access/viraccessperm.c                    |   2 +-
 src/access/viraccessperm.h                    |   6 +
 src/conf/node_device_conf.c                   |  14 +
 src/conf/node_device_conf.h                   |   8 +
 src/conf/virnodedeviceobj.c                   | 147 +++-
 src/conf/virnodedeviceobj.h                   |  24 +
 src/driver-nodedev.h                          |  14 +
 src/libvirt-nodedev.c                         | 141 +++-
 src/libvirt_private.syms                      |   6 +
 src/libvirt_public.syms                       |   3 +
 src/node_device/node_device_driver.c          | 743 +++++++++++++++++-
 src/node_device/node_device_driver.h          |  50 +-
 src/node_device/node_device_udev.c            | 217 ++++-
 src/remote/remote_driver.c                    |   3 +
 src/remote/remote_protocol.x                  |  40 +-
 src/remote_protocol-structs                   |  16 +
 src/rpc/gendispatch.pl                        |   1 +
 ...19_36ea_4111_8f0a_8c9a70e21366-define.argv |   2 +
 ...19_36ea_4111_8f0a_8c9a70e21366-define.json |   1 +
 ...019_36ea_4111_8f0a_8c9a70e21366-start.argv |   3 +-
 ...39_495e_4243_ad9f_beb3f14c23d9-define.argv |   1 +
 ...39_495e_4243_ad9f_beb3f14c23d9-define.json |   1 +
 ...16_1ca8_49ac_b176_871d16c13076-define.argv |   1 +
 ...16_1ca8_49ac_b176_871d16c13076-define.json |   1 +
 tests/nodedevmdevctldata/mdevctl-create.argv  |   1 +
 .../mdevctl-list-defined.argv                 |   1 +
 .../mdevctl-list-multiple.json                |  59 ++
 .../mdevctl-list-multiple.out.xml             |  43 +
 .../nodedevmdevctldata/mdevctl-undefine.argv  |   1 +
 tests/nodedevmdevctltest.c                    | 232 +++++-
 ...v_d069d019_36ea_4111_8f0a_8c9a70e21366.xml |   1 +
 tools/virsh-nodedev.c                         | 269 ++++++-
 36 files changed, 1931 insertions(+), 193 deletions(-)
 create mode 100644 tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9=
a70e21366-define.argv
 create mode 100644 tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9=
a70e21366-define.json
 create mode 100644 tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb=
3f14c23d9-define.argv
 create mode 100644 tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb=
3f14c23d9-define.json
 create mode 100644 tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871=
d16c13076-define.argv
 create mode 100644 tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871=
d16c13076-define.json
 create mode 100644 tests/nodedevmdevctldata/mdevctl-create.argv
 create mode 100644 tests/nodedevmdevctldata/mdevctl-list-defined.argv
 create mode 100644 tests/nodedevmdevctldata/mdevctl-list-multiple.json
 create mode 100644 tests/nodedevmdevctldata/mdevctl-list-multiple.out.xml
 create mode 100644 tests/nodedevmdevctldata/mdevctl-undefine.argv

--=20
2.26.3

On Fri, Mar 26, 2021 at 11:47:56AM -0500, Jonathon Jongsma wrote:
> This patch series follows the previously-merged series which added support for
> transient mediated devices. This series expands mdev support to include
> persistent device definitions. Again, it relies on mdevctl as the backend.
> 
> It follows the common libvirt pattern of APIs by adding the following new APIs
> for node devices:
>     - virNodeDeviceDefineXML() - defines a persistent device
>     - virNodeDeviceUndefine() - undefines a persistent device
>     - virNodeDeviceCreate() - starts a previously-defined device
> 
> It also adds virsh commands mapping to these new APIs: nodedev-define,
> nodedev-undefine, and nodedev-start.
> 
> Since we rely on mdevctl for the definition of mediated devices, we need a way
> to stay up-to-date with devices that are defined by mdevctl (outside of
> libvirt).  The method for staying up-to-date is currently a little bit crude
> due to the fact that mdevctl does not emit any events when new devices are
> added or removed. As a workaround, we create a file monitor for the mdevctl
> config directory and re-query mdevctl when we detect changes within that
> directory. In the future, mdevctl may introduce a more elegant solution.
> 
> Changes in v6:
>  - rebase to git master again
>  - remove typedefs for various *Ptr types, since they're now discouraged in
>    libvirt.

Okay, so I think I ACKed all of the patches now (if not, let me know which one
I missed). I tested the patches, found 3 leaks, one of them I mentioned here, 2
were related to the code but not a direct result of this series I believe, so
I'll tackle them some other time as a follow up.
Overall, I think we're good to push this starting with the 7.3.0 cycle.

Now, in v4 I promised to share my adjustments I made on top of your code. Some
of them you already handled yourself in v6, so I rebased and performed a bunch
of changes, so here they are:
https://gitlab.com/eskultety/libvirt/-/commits/mdev-adjustments

Note, that I only split them into multiple patches so that they're easier to
read, but I'm very well aware that probably none of them cannot be compiled
on its own (I didn't bother to be that thorough), it's just to give you an idea
what I've failed to express with words until we came to this v6. Please let me
know your opinion on the changes so that:
    a) I can send it as a *proper* follow up series
    b) you can incorporate some of the changes into your series

I don't care either way.

Regards and thanks for being patient with the review process :),
Erik

On Wed, 31 Mar 2021 16:00:48 +0200
Erik Skultety <eskultet@redhat.com> wrote:

> On Fri, Mar 26, 2021 at 11:47:56AM -0500, Jonathon Jongsma wrote:
> > This patch series follows the previously-merged series which added
> > support for transient mediated devices. This series expands mdev
> > support to include persistent device definitions. Again, it relies
> > on mdevctl as the backend.
> > 
> > It follows the common libvirt pattern of APIs by adding the
> > following new APIs for node devices:
> >     - virNodeDeviceDefineXML() - defines a persistent device
> >     - virNodeDeviceUndefine() - undefines a persistent device
> >     - virNodeDeviceCreate() - starts a previously-defined device
> > 
> > It also adds virsh commands mapping to these new APIs:
> > nodedev-define, nodedev-undefine, and nodedev-start.
> > 
> > Since we rely on mdevctl for the definition of mediated devices, we
> > need a way to stay up-to-date with devices that are defined by
> > mdevctl (outside of libvirt).  The method for staying up-to-date is
> > currently a little bit crude due to the fact that mdevctl does not
> > emit any events when new devices are added or removed. As a
> > workaround, we create a file monitor for the mdevctl config
> > directory and re-query mdevctl when we detect changes within that
> > directory. In the future, mdevctl may introduce a more elegant
> > solution.
> > 
> > Changes in v6:
> >  - rebase to git master again
> >  - remove typedefs for various *Ptr types, since they're now
> > discouraged in libvirt.  
> 
> Okay, so I think I ACKed all of the patches now (if not, let me know
> which one I missed). I tested the patches, found 3 leaks, one of them
> I mentioned here, 2 were related to the code but not a direct result
> of this series I believe, so I'll tackle them some other time as a
> follow up. Overall, I think we're good to push this starting with the
> 7.3.0 cycle.

I think everything else has been acked.

> 
> Now, in v4 I promised to share my adjustments I made on top of your
> code. Some of them you already handled yourself in v6, so I rebased
> and performed a bunch of changes, so here they are:
> https://gitlab.com/eskultety/libvirt/-/commits/mdev-adjustments
> 
> Note, that I only split them into multiple patches so that they're
> easier to read, but I'm very well aware that probably none of them
> cannot be compiled on its own (I didn't bother to be that thorough),
> it's just to give you an idea what I've failed to express with words
> until we came to this v6. Please let me know your opinion on the
> changes so that: a) I can send it as a *proper* follow up series
>     b) you can incorporate some of the changes into your series
> 

Thanks for that. I think the changes look reasonable, and I think the
benefits outweigh the drawbacks. I have a few changes I'd like to make
to your commits. See the top 3 commits at [1] for details. Would you
like me to just incorporate your stuff into my original series, or keep
them as separate commits on top of my series?

Jonathon


[1] https://gitlab.com/jjongsma/libvirt/-/commits/mdevctl-adjustments/

On Thu, Apr 01, 2021 at 10:18:45AM -0500, Jonathon Jongsma wrote:
> On Wed, 31 Mar 2021 16:00:48 +0200
> Erik Skultety <eskultet@redhat.com> wrote:
> 
> > On Fri, Mar 26, 2021 at 11:47:56AM -0500, Jonathon Jongsma wrote:
> > > This patch series follows the previously-merged series which added
> > > support for transient mediated devices. This series expands mdev
> > > support to include persistent device definitions. Again, it relies
> > > on mdevctl as the backend.
> > > 
> > > It follows the common libvirt pattern of APIs by adding the
> > > following new APIs for node devices:
> > >     - virNodeDeviceDefineXML() - defines a persistent device
> > >     - virNodeDeviceUndefine() - undefines a persistent device
> > >     - virNodeDeviceCreate() - starts a previously-defined device
> > > 
> > > It also adds virsh commands mapping to these new APIs:
> > > nodedev-define, nodedev-undefine, and nodedev-start.
> > > 
> > > Since we rely on mdevctl for the definition of mediated devices, we
> > > need a way to stay up-to-date with devices that are defined by
> > > mdevctl (outside of libvirt).  The method for staying up-to-date is
> > > currently a little bit crude due to the fact that mdevctl does not
> > > emit any events when new devices are added or removed. As a
> > > workaround, we create a file monitor for the mdevctl config
> > > directory and re-query mdevctl when we detect changes within that
> > > directory. In the future, mdevctl may introduce a more elegant
> > > solution.
> > > 
> > > Changes in v6:
> > >  - rebase to git master again
> > >  - remove typedefs for various *Ptr types, since they're now
> > > discouraged in libvirt.  
> > 
> > Okay, so I think I ACKed all of the patches now (if not, let me know
> > which one I missed). I tested the patches, found 3 leaks, one of them
> > I mentioned here, 2 were related to the code but not a direct result
> > of this series I believe, so I'll tackle them some other time as a
> > follow up. Overall, I think we're good to push this starting with the
> > 7.3.0 cycle.
> 
> I think everything else has been acked.
> 
> > 
> > Now, in v4 I promised to share my adjustments I made on top of your
> > code. Some of them you already handled yourself in v6, so I rebased
> > and performed a bunch of changes, so here they are:
> > https://gitlab.com/eskultety/libvirt/-/commits/mdev-adjustments
> > 
> > Note, that I only split them into multiple patches so that they're
> > easier to read, but I'm very well aware that probably none of them
> > cannot be compiled on its own (I didn't bother to be that thorough),
> > it's just to give you an idea what I've failed to express with words
> > until we came to this v6. Please let me know your opinion on the
> > changes so that: a) I can send it as a *proper* follow up series
> >     b) you can incorporate some of the changes into your series
> > 
> 
> Thanks for that. I think the changes look reasonable, and I think the
> benefits outweigh the drawbacks. I have a few changes I'd like to make
> to your commits. See the top 3 commits at [1] for details. Would you
> like me to just incorporate your stuff into my original series, or keep
> them as separate commits on top of my series?

I like your ^additional cleanup.

As for the 30 patches - I'd like to avoid having to go through most of them
again, so no, please go ahead an merge this series as is. The follow up changes
that started to discuss are not functional anyway, so posting them separately
is actually preferable.

As for the follow up series - now that you see what my intentions wrt
abstracting the mdevctl cmdline building code as much as possible were, feel
free to take my commits, apply your 3 changes, shuffle them around as you need,
and post it as a proper follow up series that can compile after every single
patch :) and I'll review it.

Regards,
Erik

On Wed, 7 Apr 2021 08:05:17 +0200
Erik Skultety <eskultet@redhat.com> wrote:

> On Thu, Apr 01, 2021 at 10:18:45AM -0500, Jonathon Jongsma wrote:
> > On Wed, 31 Mar 2021 16:00:48 +0200
> > Erik Skultety <eskultet@redhat.com> wrote:
> >   
> > > On Fri, Mar 26, 2021 at 11:47:56AM -0500, Jonathon Jongsma wrote:
> > >  
> > > > This patch series follows the previously-merged series which
> > > > added support for transient mediated devices. This series
> > > > expands mdev support to include persistent device definitions.
> > > > Again, it relies on mdevctl as the backend.
> > > > 
> > > > It follows the common libvirt pattern of APIs by adding the
> > > > following new APIs for node devices:
> > > >     - virNodeDeviceDefineXML() - defines a persistent device
> > > >     - virNodeDeviceUndefine() - undefines a persistent device
> > > >     - virNodeDeviceCreate() - starts a previously-defined device
> > > > 
> > > > It also adds virsh commands mapping to these new APIs:
> > > > nodedev-define, nodedev-undefine, and nodedev-start.
> > > > 
> > > > Since we rely on mdevctl for the definition of mediated
> > > > devices, we need a way to stay up-to-date with devices that are
> > > > defined by mdevctl (outside of libvirt).  The method for
> > > > staying up-to-date is currently a little bit crude due to the
> > > > fact that mdevctl does not emit any events when new devices are
> > > > added or removed. As a workaround, we create a file monitor for
> > > > the mdevctl config directory and re-query mdevctl when we
> > > > detect changes within that directory. In the future, mdevctl
> > > > may introduce a more elegant solution.
> > > > 
> > > > Changes in v6:
> > > >  - rebase to git master again
> > > >  - remove typedefs for various *Ptr types, since they're now
> > > > discouraged in libvirt.    
> > > 
> > > Okay, so I think I ACKed all of the patches now (if not, let me
> > > know which one I missed). I tested the patches, found 3 leaks,
> > > one of them I mentioned here, 2 were related to the code but not
> > > a direct result of this series I believe, so I'll tackle them
> > > some other time as a follow up. Overall, I think we're good to
> > > push this starting with the 7.3.0 cycle.  
> > 
> > I think everything else has been acked.
> >   
> > > 
> > > Now, in v4 I promised to share my adjustments I made on top of
> > > your code. Some of them you already handled yourself in v6, so I
> > > rebased and performed a bunch of changes, so here they are:
> > > https://gitlab.com/eskultety/libvirt/-/commits/mdev-adjustments
> > > 
> > > Note, that I only split them into multiple patches so that they're
> > > easier to read, but I'm very well aware that probably none of them
> > > cannot be compiled on its own (I didn't bother to be that
> > > thorough), it's just to give you an idea what I've failed to
> > > express with words until we came to this v6. Please let me know
> > > your opinion on the changes so that: a) I can send it as a
> > > *proper* follow up series b) you can incorporate some of the
> > > changes into your series 
> > 
> > Thanks for that. I think the changes look reasonable, and I think
> > the benefits outweigh the drawbacks. I have a few changes I'd like
> > to make to your commits. See the top 3 commits at [1] for details.
> > Would you like me to just incorporate your stuff into my original
> > series, or keep them as separate commits on top of my series?  
> 
> I like your ^additional cleanup.
> 
> As for the 30 patches - I'd like to avoid having to go through most
> of them again, so no, please go ahead an merge this series as is. The
> follow up changes that started to discuss are not functional anyway,
> so posting them separately is actually preferable.
> 
> As for the follow up series - now that you see what my intentions wrt
> abstracting the mdevctl cmdline building code as much as possible
> were, feel free to take my commits, apply your 3 changes, shuffle
> them around as you need, and post it as a proper follow up series
> that can compile after every single patch :) and I'll review it.

Sounds good. Will send a followup series shortly. Thanks!

Jonathon