[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt PATCH v2 10/16] api: add virNodeDeviceDefineXML()



On Tue, Aug 18, 2020 at 09:48:00AM -0500, Jonathon Jongsma wrote:
> With mediated devices, we can now define persistent node devices that
> can be started and stopped. In order to take advantage of this, we need
> an API to define new node devices.
>
> Signed-off-by: Jonathon Jongsma <jjongsma redhat com>
> ---
>  include/libvirt/libvirt-nodedev.h             |  4 +
>  src/driver-nodedev.h                          |  6 ++
>  src/libvirt-nodedev.c                         | 42 ++++++++
>  src/libvirt_public.syms                       |  4 +
>  src/node_device/node_device_driver.c          | 97 +++++++++++++++++--
>  src/node_device/node_device_driver.h          | 10 ++
>  src/node_device/node_device_udev.c            |  1 +
>  src/remote/remote_driver.c                    |  1 +
>  src/remote/remote_protocol.x                  | 18 +++-
>  src/remote_protocol-structs                   |  8 ++
>  src/rpc/gendispatch.pl                        |  1 +
>  ...19_36ea_4111_8f0a_8c9a70e21366-define.argv |  1 +
>  ...19_36ea_4111_8f0a_8c9a70e21366-define.json |  1 +
>  ...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/nodedevmdevctltest.c                    | 68 +++++++++----
>  18 files changed, 241 insertions(+), 25 deletions(-)
>  create mode 100644 tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-define.argv
>  create mode 100644 tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-define.json
>  create mode 100644 tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-define.argv
>  create mode 100644 tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-define.json
>  create mode 100644 tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-define.argv
>  create mode 100644 tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-define.json
>
> diff --git a/include/libvirt/libvirt-nodedev.h b/include/libvirt/libvirt-nodedev.h
> index 423a583d45..02aa9d9750 100644
> --- a/include/libvirt/libvirt-nodedev.h
> +++ b/include/libvirt/libvirt-nodedev.h
> @@ -127,6 +127,10 @@ virNodeDevicePtr        virNodeDeviceCreateXML  (virConnectPtr conn,
>
>  int                     virNodeDeviceDestroy    (virNodeDevicePtr dev);
>
> +virNodeDevicePtr        virNodeDeviceDefineXML  (virConnectPtr conn,
> +                                                 const char *xmlDesc,
> +                                                 unsigned int flags);

Pre-existing, but this forced space-padded alignment feels weird nowadays and
we should use the same policy as for the internal headers - no padding, 1 blank
line separating the function declarations. In fact at the end of the header,
we're breaking the padded "consistency" anyway. So, I'd suggest to abandon
the padding, use a single space as a delimiter and we can send a follow-up
to fix the rest.

the public side of things looks good to me
...

>
> +LIBVIRT_6.5.0 {
> +    global:
> +        virNodeDeviceDefineXML;
> +} LIBVIRT_6.0.0;

6.8.0 - this is just a reminder as we've managed to push new symbols
referencing an old release once IIRC :)

>  # .... define new API here using predicted next version number ....
> diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c
> index affd707a65..16f3537776 100644
> --- a/src/node_device/node_device_driver.c
> +++ b/src/node_device/node_device_driver.c
> @@ -658,9 +658,13 @@ nodeDeviceFindAddressByName(const char *name)
>  }
>
>
> -virCommandPtr
> -nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def,
> -                                 char **uuid_out)
> +/* the mdevctl 'start' and 'define' commands accept almost the exact same
> + * arguments, so provide a common implementation that can be wrapped by a more
> + * specific function */
> +static virCommandPtr
> +nodeDeviceGetMdevctlDefineStartCommand(virNodeDeviceDefPtr def,
> +                                       const char *subcommand,
> +                                       char **uuid_out)
>  {
>      virCommandPtr cmd;
>      g_autofree char *json = NULL;
> @@ -678,7 +682,7 @@ nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def,
>          return NULL;
>      }
>
> -    cmd = virCommandNewArgList(MDEVCTL, "start",
> +    cmd = virCommandNewArgList(MDEVCTL, subcommand,
>                                 "-p", parent_pci,
>                                 "--jsonfile", "/dev/stdin",
>                                 NULL);

Okay, but could we actually make ^this function the internal "public" gateway
to all the mdevctl commands accepting a larger set of arguments and...


> @@ -689,11 +693,29 @@ nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def,
>      return cmd;
>  }
>
> +virCommandPtr
> +nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def,
> +                                 char **uuid_out)

...make ^these the static implementations handling the subcommand nuances?

> +{
> +    return nodeDeviceGetMdevctlDefineStartCommand(def, "start", uuid_out);
> +}
> +
> +virCommandPtr
> +nodeDeviceGetMdevctlDefineCommand(virNodeDeviceDefPtr def,
> +                                  char **uuid_out)
> +{
> +    return nodeDeviceGetMdevctlDefineStartCommand(def, "define", uuid_out);
> +}
> +
> +
> +
>  static int
> -virMdevctlStart(virNodeDeviceDefPtr def, char **uuid)
> +virMdevctlDefineCommon(virNodeDeviceDefPtr def,
> +                       virCommandPtr (*func)(virNodeDeviceDefPtr, char**),
> +                       char **uuid)
>  {
>      int status;
> -    g_autoptr(virCommand) cmd = nodeDeviceGetMdevctlStartCommand(def, uuid);
> +    g_autoptr(virCommand) cmd = func(def, uuid);
>      if (!cmd)
>          return -1;

I'm not sure whether this standalone function brings that much value in terms
of abstracting code.

>
> @@ -709,6 +731,20 @@ virMdevctlStart(virNodeDeviceDefPtr def, char **uuid)
>  }
>

>
> +static int
> +virMdevctlStart(virNodeDeviceDefPtr def, char **uuid)
> +{
> +    return virMdevctlDefineCommon(def, nodeDeviceGetMdevctlStartCommand, uuid);

I wouldn't mind doing what virMdevctlDefineCommon does right here. In a later
patch you're introducing a virMdevctlCreate command which I think should
actually be part of this function with a bool selector whether we're starting a
transient or a persistent device.

> +}
> +
> +
> +static int
> +virMdevctlDefine(virNodeDeviceDefPtr def, char **uuid)
> +{
> +    return virMdevctlDefineCommon(def, nodeDeviceGetMdevctlDefineCommand, uuid);

here too..

[snip]

> diff --git a/tests/nodedevmdevctltest.c b/tests/nodedevmdevctltest.c
> index cee0d913dd..f821622ff6 100644
> --- a/tests/nodedevmdevctltest.c
> +++ b/tests/nodedevmdevctltest.c
> @@ -10,10 +10,16 @@
>
>  #define VIR_FROM_THIS VIR_FROM_NODEDEV
>
> +typedef enum {
> +    MDEVCTL_CMD_START,
> +    MDEVCTL_CMD_DEFINE,
> +} MdevctlCmd;

Hmm, with the suggestion I outlined above, we could define all the commands
we're going to use as an enum in node_device_driver.h like VIR_ENUM_DECL.

> +
>  struct startTestInfo {
>      const char *virt_type;
>      int create;
>      const char *filename;
> +    MdevctlCmd command;
>  };
>
>  /* capture stdin passed to command */
> @@ -45,12 +51,17 @@ nodedevCompareToFile(const char *actual,
>      return virTestCompareToFile(replacedCmdline, filename);
>  }
>
> +
> +typedef virCommandPtr (*GetStartDefineCmdFunc)(virNodeDeviceDefPtr, char **);
> +
> +
>  static int
>  testMdevctlStart(const char *virt_type,
>                   int create,
> +                 GetStartDefineCmdFunc get_cmd_func,
>                   const char *mdevxml,
> -                 const char *startcmdfile,
> -                 const char *startjsonfile)
> +                 const char *cmdfile,
> +                 const char *jsonfile)
>  {
>      g_autoptr(virNodeDeviceDef) def = NULL;
>      virNodeDeviceObjPtr obj = NULL;
> @@ -66,7 +77,7 @@ testMdevctlStart(const char *virt_type,
>
>      /* this function will set a stdin buffer containing the json configuration
>       * of the device. The json value is captured in the callback above */
> -    cmd = nodeDeviceGetMdevctlStartCommand(def, &uuid);
> +    cmd = get_cmd_func(def, &uuid);
>
>      if (!cmd)
>          goto cleanup;
> @@ -78,10 +89,10 @@ testMdevctlStart(const char *virt_type,
>      if (!(actualCmdline = virBufferCurrentContent(&buf)))
>          goto cleanup;
>
> -    if (nodedevCompareToFile(actualCmdline, startcmdfile) < 0)
> +    if (nodedevCompareToFile(actualCmdline, cmdfile) < 0)
>          goto cleanup;
>
> -    if (virTestCompareToFile(stdinbuf, startjsonfile) < 0)
> +    if (virTestCompareToFile(stdinbuf, jsonfile) < 0)
>          goto cleanup;
>
>      ret = 0;
> @@ -96,17 +107,31 @@ static int
>  testMdevctlStartHelper(const void *data)
>  {
>      const struct startTestInfo *info = data;
> +    const char *cmd;
> +    GetStartDefineCmdFunc func;
> +    g_autofree char *mdevxml = NULL;
> +    g_autofree char *cmdlinefile = NULL;
> +    g_autofree char *jsonfile = NULL;
> +
> +    if (info->command == MDEVCTL_CMD_START) {
> +        cmd = "start";
> +        func = nodeDeviceGetMdevctlStartCommand;
> +    } else if (info->command == MDEVCTL_CMD_DEFINE) {
> +        cmd = "define";
> +        func = nodeDeviceGetMdevctlDefineCommand;
> +    } else {

^This would then not be needed and we'd simply call
virMdevctlGetCommand(def, MDEVCTL_COMMAND_<SUBCOMMAND>, ...)


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]