[libvirt] [PATCH] storage_backend_rbd: check the return value of rados_conf_set

[Peter Krempa]

On Fri, Nov 11, 2016 at 17:54:57 +0800, Chen Hanxiao wrote:
> At 2016-11-11 17:27:48, "Peter Krempa" <pkrempa at redhat.com> wrote:
> >On Fri, Nov 11, 2016 at 16:33:18 +0800, Chen Hanxiao wrote:
> >> From: Chen Hanxiao <chenhanxiao at gmail.com>
> >> 
> >> We forget to check the return value of rados_conf_set.
> >> 
> >> Signed-off-by: Chen Hanxiao <chenhanxiao at gmail.com>
> >> ---
> >>  src/storage/storage_backend_rbd.c | 21 ++++++++++++++++++---
> >>  1 file changed, 18 insertions(+), 3 deletions(-)

[...]

> >>      VIR_DEBUG("Setting RADOS option rados_mon_op_timeout to %s", mon_op_timeout);
> >> -    rados_conf_set(ptr->cluster, "rados_mon_op_timeout", mon_op_timeout);
> >> +    if (rados_conf_set(ptr->cluster, "rados_mon_op_timeout", mon_op_timeout) < 0) {
> >> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> >> +                       _("failed to set RADOS option: %s"),
> >> +                       "rados_mon_op_timeout");
> >> +        goto cleanup;
> >> +    }
> >
> >Did you have any problems with this? The documentation mentions only one
> 
> In a test, I failed in rados_connect once.
> When I try again, it works.

That looks more like a rados or network problem.

> So I wonder maybe something wrong in rados_conf_set.

That is very unlikely.

> 
> >error code (ENOENT) if the given config option does not exist in
> >librados. This would point to us something doing wrong rather than a
> >random failure and we should address the primary cause.
> 
> Any hints?

I'd try to debug why rados_connect failed in the first place. The
options you added debug statements for don't look critical enough.

Said that, the patch itself is reasonable if you add error reporting for
all the config options we set.

Peter
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20161111/2709651b/attachment-0001.sig>