[libvirt] [PATCH v2 8/8] storage: Replace VIR_ERROR with standard vir*Error in state driver init

[Cole Robinson]

On 05/23/2016 02:36 PM, Jovanka Gulicoska wrote:
> Replace VIR_ERROR with virReportError and virReportSystemError
> ---
>  src/storage/storage_driver.c | 32 ++++++++++++++++++++------------
>  1 file changed, 20 insertions(+), 12 deletions(-)
> 
> diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
> index eb5f688..879b3c7 100644
> --- a/src/storage/storage_driver.c
> +++ b/src/storage/storage_driver.c
> @@ -88,7 +88,8 @@ storagePoolUpdateState(virStoragePoolObjPtr pool)
>          goto error;
>  
>      if ((backend = virStorageBackendForType(pool->def->type)) == NULL) {
> -        VIR_ERROR(_("Missing backend %d"), pool->def->type);
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("Missing backend %d"), pool->def->type);
>          goto error;
>      }
>  
> @@ -98,8 +99,9 @@ storagePoolUpdateState(virStoragePoolObjPtr pool)
>      active = false;
>      if (backend->checkPool &&
>          backend->checkPool(pool, &active) < 0) {
> -        VIR_ERROR(_("Failed to initialize storage pool '%s': %s"),
> -                  pool->def->name, virGetLastErrorMessage());
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("Failed to initialize storage pool '%s': %s"),
> +                       pool->def->name, virGetLastErrorMessage());
>          goto error;
>      }
>  
> @@ -112,8 +114,9 @@ storagePoolUpdateState(virStoragePoolObjPtr pool)
>          if (backend->refreshPool(NULL, pool) < 0) {
>              if (backend->stopPool)
>                  backend->stopPool(NULL, pool);
> -            VIR_ERROR(_("Failed to restart storage pool '%s': %s"),
> -                      pool->def->name, virGetLastErrorMessage());
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("Failed to restart storage pool '%s': %s"),
> +                           pool->def->name, virGetLastErrorMessage());
>              goto error;
>          }
>      }
> @@ -172,8 +175,9 @@ storageDriverAutostart(void)
>              !virStoragePoolObjIsActive(pool)) {
>              if (backend->startPool &&
>                  backend->startPool(conn, pool) < 0) {
> -                VIR_ERROR(_("Failed to autostart storage pool '%s': %s"),
> -                          pool->def->name, virGetLastErrorMessage());
> +                virReportError(VIR_ERR_INTERNAL_ERROR,
> +                               _("Failed to autostart storage pool '%s': %s"),
> +                               pool->def->name, virGetLastErrorMessage());
>                  virStoragePoolObjUnlock(pool);
>                  continue;
>              }
> @@ -193,8 +197,9 @@ storageDriverAutostart(void)
>                      unlink(stateFile);
>                  if (backend->stopPool)
>                      backend->stopPool(conn, pool);
> -                VIR_ERROR(_("Failed to autostart storage pool '%s': %s"),
> -                          pool->def->name, virGetLastErrorMessage());
> +                virReportError(VIR_ERR_INTERNAL_ERROR,
> +                               _("Failed to autostart storage pool '%s': %s"),
> +                               pool->def->name, virGetLastErrorMessage());
>              } else {
>                  pool->active = true;
>              }

The above changes look good, and I pushed this patch with these bottom two
bits dropped.

> @@ -835,8 +840,10 @@ storagePoolUndefine(virStoragePoolPtr obj)
>          errno != ENOENT &&
>          errno != ENOTDIR) {
>          char ebuf[1024];
> -        VIR_ERROR(_("Failed to delete autostart link '%s': %s"),
> -                  pool->autostartLink, virStrerror(errno, ebuf, sizeof(ebuf)));
> +        virReportSystemError(errno,
> +                             _("Failed to delete autostart link '%s': %s"),
> +                             pool->autostartLink,
> +                             virStrerror(errno, ebuf, sizeof(ebuf)));
>      }
>  

This has the SystemError issue I outlined elsewhere, but actually this call
site is a legitimate use of VIR_ERROR. storagePoolUndefine is called via
direct user interaction, like virsh pool-undefine. In those cases, we only
want to call virReport*Error if we hit a fatal error (cause the function to
return an exit code). In this case, we want to log the failure, but not fail
the API call over it, so VIR_ERROR is more appropriate IMO

>      VIR_FREE(pool->configFile);
> @@ -2301,7 +2308,8 @@ virStorageVolFDStreamCloseCb(virStreamPtr st ATTRIBUTE_UNUSED,
>      if (virThreadCreate(&thread, false, virStorageVolPoolRefreshThread,
>                          opaque) < 0) {
>          /* Not much else can be done */
> -        VIR_ERROR(_("Failed to create thread to handle pool refresh"));
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("Failed to create thread to handle pool refresh"));
>          goto error;
>      }
>      return; /* Thread will free opaque data */
> 

This is basically a similar case. Though this case will cause the function to
fail, it's used as an async callback and not something via direct user
interaction, so it's probably better to not mess with actual error reporting.
It may make sense to convert to virReportError but would need closer inspection.

Thanks,
Cole