[libvirt] [PATCH] storage_backend_rbd: check the return value of rados_conf_set
From: Chen Hanxiao <chenhanxiao@gmail.com> We forget to check the return value of rados_conf_set. Signed-off-by: Chen Hanxiao <chenhanxiao@gmail.com> --- src/storage/storage_backend_rbd.c | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c index 718c4d6..58bcb9a 100644 --- a/src/storage/storage_backend_rbd.c +++ b/src/storage/storage_backend_rbd.c @@ -162,10 +162,20 @@ virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr, rados_conf_set(ptr->cluster, "client_mount_timeout", client_mount_timeout); 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; + } VIR_DEBUG("Setting RADOS option rados_osd_op_timeout to %s", osd_op_timeout); - rados_conf_set(ptr->cluster, "rados_osd_op_timeout", osd_op_timeout); + if (rados_conf_set(ptr->cluster, "rados_osd_op_timeout", osd_op_timeout) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("failed to set RADOS option: %s"), + "rados_osd_op_timeout"); + goto cleanup; + } /* * Librbd supports creating RBD format 2 images. We no longer have to invoke @@ -173,7 +183,12 @@ virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr, * This leaves us to simply use rbd_create() and use the default behavior of librbd */ VIR_DEBUG("Setting RADOS option rbd_default_format to %s", rbd_default_format); - rados_conf_set(ptr->cluster, "rbd_default_format", rbd_default_format); + if (rados_conf_set(ptr->cluster, "rbd_default_format", rbd_default_format) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("failed to set RADOS option: %s"), + "rbd_default_format"); + goto cleanup; + } ptr->starttime = time(0); if ((r = rados_connect(ptr->cluster)) < 0) { -- 1.8.3.1
On Fri, Nov 11, 2016 at 16:33:18 +0800, Chen Hanxiao wrote:
From: Chen Hanxiao <chenhanxiao@gmail.com>
We forget to check the return value of rados_conf_set.
Signed-off-by: Chen Hanxiao <chenhanxiao@gmail.com> --- src/storage/storage_backend_rbd.c | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-)
diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c index 718c4d6..58bcb9a 100644 --- a/src/storage/storage_backend_rbd.c +++ b/src/storage/storage_backend_rbd.c @@ -162,10 +162,20 @@ virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr, rados_conf_set(ptr->cluster, "client_mount_timeout", client_mount_timeout);
Right in this context is one instance that is not changed ^^^.
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 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. Peter
At 2016-11-11 17:27:48, "Peter Krempa" <pkrempa@redhat.com> wrote:
On Fri, Nov 11, 2016 at 16:33:18 +0800, Chen Hanxiao wrote:
From: Chen Hanxiao <chenhanxiao@gmail.com>
We forget to check the return value of rados_conf_set.
Signed-off-by: Chen Hanxiao <chenhanxiao@gmail.com> --- src/storage/storage_backend_rbd.c | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-)
diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c index 718c4d6..58bcb9a 100644 --- a/src/storage/storage_backend_rbd.c +++ b/src/storage/storage_backend_rbd.c @@ -162,10 +162,20 @@ virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr, rados_conf_set(ptr->cluster, "client_mount_timeout", client_mount_timeout);
Right in this context is one instance that is not changed ^^^.
Oops...
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. So I wonder maybe something wrong in rados_conf_set.
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? Regards, - Chen
On Fri, Nov 11, 2016 at 17:54:57 +0800, Chen Hanxiao wrote:
At 2016-11-11 17:27:48, "Peter Krempa" <pkrempa@redhat.com> wrote:
On Fri, Nov 11, 2016 at 16:33:18 +0800, Chen Hanxiao wrote:
From: Chen Hanxiao <chenhanxiao@gmail.com>
We forget to check the return value of rados_conf_set.
Signed-off-by: Chen Hanxiao <chenhanxiao@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
At 2016-11-11 18:30:05, "Peter Krempa" <pkrempa@redhat.com> wrote:
On Fri, Nov 11, 2016 at 17:54:57 +0800, Chen Hanxiao wrote:
At 2016-11-11 17:27:48, "Peter Krempa" <pkrempa@redhat.com> wrote:
On Fri, Nov 11, 2016 at 16:33:18 +0800, Chen Hanxiao wrote:
From: Chen Hanxiao <chenhanxiao@gmail.com>
We forget to check the return value of rados_conf_set.
Signed-off-by: Chen Hanxiao <chenhanxiao@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.
Thanks for your advice. I'll try to debug that. v2 will come soon. Regards, - Chen
participants (2)
-
Chen Hanxiao -
Peter Krempa