diff mbox

[08/20] gluster: correctly propagate errors

Message ID 1391939335-31580-9-git-send-email-pbonzini@redhat.com
State New
Headers show

Commit Message

Paolo Bonzini Feb. 9, 2014, 9:48 a.m. UTC 
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/gluster.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

Comments

Fam Zheng Feb. 10, 2014, 8:02 a.m. UTC | #1 
On Sun, 02/09 10:48, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  block/gluster.c | 24 +++++++++++++-----------
>  1 file changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/block/gluster.c b/block/gluster.c
> index f9dd37f..bc9c59f 100644
> --- a/block/gluster.c
> +++ b/block/gluster.c
> @@ -175,7 +175,8 @@ out:
>      return ret;
>  }
>  
> -static struct glfs *qemu_gluster_init(GlusterConf *gconf, const char *filename)
> +static struct glfs *qemu_gluster_init(GlusterConf *gconf, const char *filename,
> +                                      Error **errp)
>  {
>      struct glfs *glfs = NULL;
>      int ret;
> @@ -183,8 +184,8 @@ static struct glfs *qemu_gluster_init(GlusterConf *gconf, const char *filename)
>  
>      ret = qemu_gluster_parseuri(gconf, filename);
>      if (ret < 0) {
> -        error_report("Usage: file=gluster[+transport]://[server[:port]]/"
> -            "volname/image[?socket=...]");
> +        error_setg(errp, "Usage: file=gluster[+transport]://[server[:port]]/"
> +                   "volname/image[?socket=...]");
>          errno = -ret;
>          goto out;
>      }
> @@ -211,9 +212,11 @@ static struct glfs *qemu_gluster_init(GlusterConf *gconf, const char *filename)
>  
>      ret = glfs_init(glfs);
>      if (ret) {
> -        error_report("Gluster connection failed for server=%s port=%d "
> -             "volume=%s image=%s transport=%s", gconf->server, gconf->port,
> -             gconf->volname, gconf->image, gconf->transport);
> +        error_setg_errno(errp, errno,
> +                         "Gluster connection failed for server=%s port=%d "
> +                         "volume=%s image=%s transport=%s", gconf->server,
> +                         gconf->port, gconf->volname, gconf->image,
> +                         gconf->transport);
>          goto out;
>      }
>      return glfs;
> @@ -283,15 +286,14 @@ static int qemu_gluster_open(BlockDriverState *bs,  QDict *options,
>      opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
>      qemu_opts_absorb_qdict(opts, options, &local_err);
>      if (error_is_set(&local_err)) {
> -        qerror_report_err(local_err);
> -        error_free(local_err);
> +        error_propagate(errp, local_err);
>          ret = -EINVAL;
>          goto out;
>      }
>  
>      filename = qemu_opt_get(opts, "filename");
>  
> -    s->glfs = qemu_gluster_init(gconf, filename);
> +    s->glfs = qemu_gluster_init(gconf, filename, errp);
>      if (!s->glfs) {
>          ret = -errno;
>          goto out;
> @@ -389,9 +391,9 @@ static int qemu_gluster_create(const char *filename,
>      int64_t total_size = 0;
>      GlusterConf *gconf = g_malloc0(sizeof(GlusterConf));
>  
> -    glfs = qemu_gluster_init(gconf, filename);
> +    glfs = qemu_gluster_init(gconf, filename, errp);
>      if (!glfs) {
> -        ret = -errno;
> +        ret = -EINVAL;

Why dropping -errno here?

Thanks,
Fam
Paolo Bonzini Feb. 10, 2014, 8:27 a.m. UTC | #2 
Il 10/02/2014 09:02, Fam Zheng ha scritto:
> On Sun, 02/09 10:48, Paolo Bonzini wrote:
>> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> > ---
>> >  block/gluster.c | 24 +++++++++++++-----------
>> >  1 file changed, 13 insertions(+), 11 deletions(-)
>> >
>> > diff --git a/block/gluster.c b/block/gluster.c
>> > index f9dd37f..bc9c59f 100644
>> > --- a/block/gluster.c
>> > +++ b/block/gluster.c
>> > @@ -175,7 +175,8 @@ out:
>> >      return ret;
>> >  }
>> >
>> > -static struct glfs *qemu_gluster_init(GlusterConf *gconf, const char *filename)
>> > +static struct glfs *qemu_gluster_init(GlusterConf *gconf, const char *filename,
>> > +                                      Error **errp)
>> >  {
>> >      struct glfs *glfs = NULL;
>> >      int ret;
>> > @@ -183,8 +184,8 @@ static struct glfs *qemu_gluster_init(GlusterConf *gconf, const char *filename)
>> >
>> >      ret = qemu_gluster_parseuri(gconf, filename);
>> >      if (ret < 0) {
>> > -        error_report("Usage: file=gluster[+transport]://[server[:port]]/"
>> > -            "volname/image[?socket=...]");
>> > +        error_setg(errp, "Usage: file=gluster[+transport]://[server[:port]]/"
>> > +                   "volname/image[?socket=...]");
>> >          errno = -ret;
>> >          goto out;
>> >      }
>> > @@ -211,9 +212,11 @@ static struct glfs *qemu_gluster_init(GlusterConf *gconf, const char *filename)
>> >
>> >      ret = glfs_init(glfs);
>> >      if (ret) {
>> > -        error_report("Gluster connection failed for server=%s port=%d "
>> > -             "volume=%s image=%s transport=%s", gconf->server, gconf->port,
>> > -             gconf->volname, gconf->image, gconf->transport);
>> > +        error_setg_errno(errp, errno,
>> > +                         "Gluster connection failed for server=%s port=%d "
>> > +                         "volume=%s image=%s transport=%s", gconf->server,
>> > +                         gconf->port, gconf->volname, gconf->image,
>> > +                         gconf->transport);
>> >          goto out;
>> >      }
>> >      return glfs;
>> > @@ -283,15 +286,14 @@ static int qemu_gluster_open(BlockDriverState *bs,  QDict *options,
>> >      opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
>> >      qemu_opts_absorb_qdict(opts, options, &local_err);
>> >      if (error_is_set(&local_err)) {
>> > -        qerror_report_err(local_err);
>> > -        error_free(local_err);
>> > +        error_propagate(errp, local_err);
>> >          ret = -EINVAL;
>> >          goto out;
>> >      }
>> >
>> >      filename = qemu_opt_get(opts, "filename");
>> >
>> > -    s->glfs = qemu_gluster_init(gconf, filename);
>> > +    s->glfs = qemu_gluster_init(gconf, filename, errp);
>> >      if (!s->glfs) {
>> >          ret = -errno;
>> >          goto out;
>> > @@ -389,9 +391,9 @@ static int qemu_gluster_create(const char *filename,
>> >      int64_t total_size = 0;
>> >      GlusterConf *gconf = g_malloc0(sizeof(GlusterConf));
>> >
>> > -    glfs = qemu_gluster_init(gconf, filename);
>> > +    glfs = qemu_gluster_init(gconf, filename, errp);
>> >      if (!glfs) {
>> > -        ret = -errno;
>> > +        ret = -EINVAL;
> Why dropping -errno here?

Because it is not applicable for the usage error messages.  Anyway 
nothing is lost because the errno is already embedded in the error if 
applicable.  If the Error* is set to non-NULL, the caller of 
drv->bdrv_open ignores the return value except to check that it is <0.

Paolo
Fam Zheng Feb. 10, 2014, 9:13 a.m. UTC | #3 
On Mon, 02/10 09:27, Paolo Bonzini wrote:
> Il 10/02/2014 09:02, Fam Zheng ha scritto:
> >On Sun, 02/09 10:48, Paolo Bonzini wrote:
> >>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> >>> ---
> >>>  block/gluster.c | 24 +++++++++++++-----------
> >>>  1 file changed, 13 insertions(+), 11 deletions(-)
> >>>
> >>> diff --git a/block/gluster.c b/block/gluster.c
> >>> index f9dd37f..bc9c59f 100644
> >>> --- a/block/gluster.c
> >>> +++ b/block/gluster.c
> >>> @@ -175,7 +175,8 @@ out:
> >>>      return ret;
> >>>  }
> >>>
> >>> -static struct glfs *qemu_gluster_init(GlusterConf *gconf, const char *filename)
> >>> +static struct glfs *qemu_gluster_init(GlusterConf *gconf, const char *filename,
> >>> +                                      Error **errp)
> >>>  {
> >>>      struct glfs *glfs = NULL;
> >>>      int ret;
> >>> @@ -183,8 +184,8 @@ static struct glfs *qemu_gluster_init(GlusterConf *gconf, const char *filename)
> >>>
> >>>      ret = qemu_gluster_parseuri(gconf, filename);
> >>>      if (ret < 0) {
> >>> -        error_report("Usage: file=gluster[+transport]://[server[:port]]/"
> >>> -            "volname/image[?socket=...]");
> >>> +        error_setg(errp, "Usage: file=gluster[+transport]://[server[:port]]/"
> >>> +                   "volname/image[?socket=...]");
> >>>          errno = -ret;
> >>>          goto out;
> >>>      }
> >>> @@ -211,9 +212,11 @@ static struct glfs *qemu_gluster_init(GlusterConf *gconf, const char *filename)
> >>>
> >>>      ret = glfs_init(glfs);
> >>>      if (ret) {
> >>> -        error_report("Gluster connection failed for server=%s port=%d "
> >>> -             "volume=%s image=%s transport=%s", gconf->server, gconf->port,
> >>> -             gconf->volname, gconf->image, gconf->transport);
> >>> +        error_setg_errno(errp, errno,
> >>> +                         "Gluster connection failed for server=%s port=%d "
> >>> +                         "volume=%s image=%s transport=%s", gconf->server,
> >>> +                         gconf->port, gconf->volname, gconf->image,
> >>> +                         gconf->transport);
> >>>          goto out;
> >>>      }
> >>>      return glfs;
> >>> @@ -283,15 +286,14 @@ static int qemu_gluster_open(BlockDriverState *bs,  QDict *options,
> >>>      opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
> >>>      qemu_opts_absorb_qdict(opts, options, &local_err);
> >>>      if (error_is_set(&local_err)) {
> >>> -        qerror_report_err(local_err);
> >>> -        error_free(local_err);
> >>> +        error_propagate(errp, local_err);
> >>>          ret = -EINVAL;
> >>>          goto out;
> >>>      }
> >>>
> >>>      filename = qemu_opt_get(opts, "filename");
> >>>
> >>> -    s->glfs = qemu_gluster_init(gconf, filename);
> >>> +    s->glfs = qemu_gluster_init(gconf, filename, errp);
> >>>      if (!s->glfs) {
> >>>          ret = -errno;
> >>>          goto out;
> >>> @@ -389,9 +391,9 @@ static int qemu_gluster_create(const char *filename,
> >>>      int64_t total_size = 0;
> >>>      GlusterConf *gconf = g_malloc0(sizeof(GlusterConf));
> >>>
> >>> -    glfs = qemu_gluster_init(gconf, filename);
> >>> +    glfs = qemu_gluster_init(gconf, filename, errp);
> >>>      if (!glfs) {
> >>> -        ret = -errno;
> >>> +        ret = -EINVAL;
> >Why dropping -errno here?
> 
> Because it is not applicable for the usage error messages.  Anyway nothing
> is lost because the errno is already embedded in the error if applicable.
> If the Error* is set to non-NULL, the caller of drv->bdrv_open ignores the
> return value except to check that it is <0.
> 

OK, then,

Reviewed-by: Fam Zheng <famz@redhat.com>
diff mbox

Patch

diff --git a/block/gluster.c b/block/gluster.c
index f9dd37f..bc9c59f 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -175,7 +175,8 @@  out:
     return ret;
 }
 
-static struct glfs *qemu_gluster_init(GlusterConf *gconf, const char *filename)
+static struct glfs *qemu_gluster_init(GlusterConf *gconf, const char *filename,
+                                      Error **errp)
 {
     struct glfs *glfs = NULL;
     int ret;
@@ -183,8 +184,8 @@  static struct glfs *qemu_gluster_init(GlusterConf *gconf, const char *filename)
 
     ret = qemu_gluster_parseuri(gconf, filename);
     if (ret < 0) {
-        error_report("Usage: file=gluster[+transport]://[server[:port]]/"
-            "volname/image[?socket=...]");
+        error_setg(errp, "Usage: file=gluster[+transport]://[server[:port]]/"
+                   "volname/image[?socket=...]");
         errno = -ret;
         goto out;
     }
@@ -211,9 +212,11 @@  static struct glfs *qemu_gluster_init(GlusterConf *gconf, const char *filename)
 
     ret = glfs_init(glfs);
     if (ret) {
-        error_report("Gluster connection failed for server=%s port=%d "
-             "volume=%s image=%s transport=%s", gconf->server, gconf->port,
-             gconf->volname, gconf->image, gconf->transport);
+        error_setg_errno(errp, errno,
+                         "Gluster connection failed for server=%s port=%d "
+                         "volume=%s image=%s transport=%s", gconf->server,
+                         gconf->port, gconf->volname, gconf->image,
+                         gconf->transport);
         goto out;
     }
     return glfs;
@@ -283,15 +286,14 @@  static int qemu_gluster_open(BlockDriverState *bs,  QDict *options,
     opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
     qemu_opts_absorb_qdict(opts, options, &local_err);
     if (error_is_set(&local_err)) {
-        qerror_report_err(local_err);
-        error_free(local_err);
+        error_propagate(errp, local_err);
         ret = -EINVAL;
         goto out;
     }
 
     filename = qemu_opt_get(opts, "filename");
 
-    s->glfs = qemu_gluster_init(gconf, filename);
+    s->glfs = qemu_gluster_init(gconf, filename, errp);
     if (!s->glfs) {
         ret = -errno;
         goto out;
@@ -389,9 +391,9 @@  static int qemu_gluster_create(const char *filename,
     int64_t total_size = 0;
     GlusterConf *gconf = g_malloc0(sizeof(GlusterConf));
 
-    glfs = qemu_gluster_init(gconf, filename);
+    glfs = qemu_gluster_init(gconf, filename, errp);
     if (!glfs) {
-        ret = -errno;
+        ret = -EINVAL;
         goto out;
     }