On Tue, Jun 15, 2021 at 11:28:40 -0500, Jonathon Jongsma wrote:
> Currently, we have two different types of mdevctl errors:
>  1. the command cannot be executed due to some error
>  2. the command is executed, but returns an error status
> 
> These two different failures are handled differently. Scenario 1 calls
> virReportError() from within nodeDeviceGetMdevctlCommand() and returns
> an error status (-1). Scenario 2 also returns -1, but does not call
> virReportError() and instead passes the error message back in the
> errmsg argument.
> 
> This means that the caller has to check both whether the return value is
> negative and whether the errmsg parameter is non-NULL before deciding
> whether to report the error or not. The situation is further complicated
> by the fact that there are occasional instances where mdevctl exits with
> an error status but does not print an error message.  This results in
> errmsg being an empty string "" (i.e. non-NULL).
> 
> Simplify the situation by not calling virReportError() at all from
> within nodeDeviceGetMdevctlCommand() and instead returning an error
> message for those that previously called virReportError(). The caller is
> now always responsible for reporting the error.
> 
> Also introduce a simple macro that converts NULL or empty errmsg to
> "Unknown Error".
> 
> Signed-off-by: Jonathon Jongsma <jjong...@redhat.com>
> ---
>  src/node_device/node_device_driver.c | 34 ++++++++++++++--------------
>  1 file changed, 17 insertions(+), 17 deletions(-)
> 
> diff --git a/src/node_device/node_device_driver.c 
> b/src/node_device/node_device_driver.c
> index e6d4e6ccb1..3cf9fc129f 100644
> --- a/src/node_device/node_device_driver.c
> +++ b/src/node_device/node_device_driver.c
> @@ -752,14 +752,13 @@ nodeDeviceGetMdevctlCommand(virNodeDeviceDef *def,
>          parent_addr = nodeDeviceFindAddressByName(def->parent);
>  
>          if (!parent_addr) {
> -            virReportError(VIR_ERR_INTERNAL_ERROR,
> -                           _("unable to find parent device '%s'"), 
> def->parent);
> +            *errbuf = g_strdup_printf(_("unable to find parent device '%s'"),
> +                                      def->parent);
>              return NULL;
>          }
>  
>          if (nodeDeviceDefToMdevctlConfig(def, &inbuf) < 0) {
> -            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -                           _("couldn't convert node device def to mdevctl 
> JSON"));
> +            *errbuf = g_strdup(_("couldn't convert node device def to 
> mdevctl JSON"));
>              return NULL;
>          }

I'm not sure whether this works properly with translations.

>  
> @@ -832,6 +831,9 @@ virMdevctlDefine(virNodeDeviceDef *def, char **uuid, char 
> **errmsg)
>  }
>  
>  
> +#define MDEVCTL_ERROR(msg) msg && msg[0] != '\0' ? msg : _("Unknown error")
> +
> +
>  static virNodeDevicePtr
>  nodeDeviceCreateXMLMdev(virConnectPtr conn,
>                          virNodeDeviceDef *def)
> @@ -846,10 +848,9 @@ nodeDeviceCreateXMLMdev(virConnectPtr conn,
>      }
>  
>      if (virMdevctlCreate(def, &uuid, &errmsg) < 0) {
> -        if (errmsg)
> -            virReportError(VIR_ERR_INTERNAL_ERROR,
> -                           _("Unable to start mediated device: %s"),
> -                           errmsg);
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("Unable to start mediated device: %s"),
> +                       MDEVCTL_ERROR(errmsg));
>          return NULL;
>      }
>  
> @@ -1201,10 +1202,9 @@ nodeDeviceDestroy(virNodeDevicePtr device)
>          }
>  
>          if (virMdevctlStop(def, &errmsg) < 0) {
> -            if (errmsg)
> -                virReportError(VIR_ERR_INTERNAL_ERROR,
> -                               _("Unable to destroy '%s': %s"), def->name,
> -                               errmsg);
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("Unable to destroy '%s': %s"), def->name,

This error message should contain also "mediated device" so that it's
easier to translate.

> +                           MDEVCTL_ERROR(errmsg));
>              goto cleanup;
>          }
>          ret = 0;

[...]

> @@ -1372,7 +1372,7 @@ nodeDeviceUndefine(virNodeDevice *device,
>          if (virMdevctlUndefine(def, &errmsg) < 0) {
>              virReportError(VIR_ERR_INTERNAL_ERROR,
>                             _("Unable to undefine mediated device: %s"),
> -                           errmsg && errmsg[0] ? errmsg : "Unknown Error");
> +                           MDEVCTL_ERROR(errmsg));
>              goto cleanup;
>          }
>          ret = 0;
> @@ -1419,7 +1419,7 @@ nodeDeviceCreate(virNodeDevice *device,
>          if (virMdevctlStart(def, &errmsg) < 0) {
>              virReportError(VIR_ERR_INTERNAL_ERROR,
>                             _("Unable to create mediated device: %s"),
> -                           errmsg && errmsg[0] ? errmsg : "Unknown Error");
> +                           MDEVCTL_ERROR(errmsg));
>              goto cleanup;
>          }
>          ret = 0;

These two almost look like a separate refactor.

Reply via email to