[libvirt] [PATCH 0/4] fix virReportSystemError misuse
virReportSystemError() reports some OS errors, and the first argument of it should be the error number defined in errno.h. virReportError() reports libvirt errors, and the first argument should be the error number defined in virerrno.h. This patch set fix some misuse of virReportSystemError: passing virerrno instead of errno. Jincheng Miao (4): qemu: fix wrong errno report in qemuMonitorOpenUnix lxc: print ENOTSUP when usernamespace is not supported openvz: print EOVERFLOW when barrier:limit are too long util: print errno in virObjectLockableNew src/lxc/lxc_container.c | 2 +- src/openvz/openvz_conf.c | 2 +- src/qemu/qemu_monitor.c | 7 +++++-- src/util/virobject.c | 2 +- 4 files changed, 8 insertions(+), 5 deletions(-) -- 1.8.3.1
In qemuMonitorOpenUnix, after connect(), virProcessKill will be invoked when cpid is not empty. But if the qemu-kvm process exited early, virProcessKill will flush errno as ESRCH, so the wrong message will be recorded in log as: error: qemuMonitorOpenUnix:309 : failed to connect to monitor socket: No such process After patched: error : qemuMonitorOpenUnix:312 : failed to connect to monitor socket: Connection refused Signed-off-by: Jincheng Miao <jmiao@redhat.com> --- src/qemu/qemu_monitor.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index db3dd73..c8284fe 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -293,19 +293,22 @@ qemuMonitorOpenUnix(const char *monitor, pid_t cpid) } do { + int orig_errno; ret = connect(monfd, (struct sockaddr *) &addr, sizeof(addr)); if (ret == 0) break; - if ((errno == ENOENT || errno == ECONNREFUSED) && + orig_errno = errno; + + if ((orig_errno == ENOENT || orig_errno == ECONNREFUSED) && (!cpid || virProcessKill(cpid, 0) == 0)) { /* ENOENT : Socket may not have shown up yet * ECONNREFUSED : Leftover socket hasn't been removed yet */ continue; } - virReportSystemError(errno, "%s", + virReportSystemError(orig_errno, "%s", _("failed to connect to monitor socket")); goto error; -- 1.8.3.1
On 07/15/2014 02:32 AM, Jincheng Miao wrote:
In qemuMonitorOpenUnix, after connect(), virProcessKill will be invoked when cpid is not empty. But if the qemu-kvm process exited early, virProcessKill will flush errno as ESRCH, so the wrong message will be recorded in log as: error: qemuMonitorOpenUnix:309 : failed to connect to monitor socket: No such process
After patched: error : qemuMonitorOpenUnix:312 : failed to connect to monitor socket: Connection refused
Signed-off-by: Jincheng Miao <jmiao@redhat.com> --- src/qemu/qemu_monitor.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index db3dd73..c8284fe 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -293,19 +293,22 @@ qemuMonitorOpenUnix(const char *monitor, pid_t cpid) }
do { + int orig_errno; ret = connect(monfd, (struct sockaddr *) &addr, sizeof(addr));
if (ret == 0) break;
- if ((errno == ENOENT || errno == ECONNREFUSED) && + orig_errno = errno; + + if ((orig_errno == ENOENT || orig_errno == ECONNREFUSED) && (!cpid || virProcessKill(cpid, 0) == 0)) {
virProcessKill can be called on cleanup paths. I think it might be wiser to rewrite virProcessKill() to guarantee that it does not modify errno than it is to make all callers have to store a temporary. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
----- Original Message -----
virProcessKill can be called on cleanup paths. I think it might be wiser to rewrite virProcessKill() to guarantee that it does not modify errno than it is to make all callers have to store a temporary.
Hi Eric, this virProcessKill only test whether this process is alive, I think it shouldn't be moved to cleanup path. :P Yes, this is a special use case of virProcessKill in qemuMonitorOpenUnix. But for other use case of virProcessKill, I think errno is really needed, some useful error information will be stored to errno: EINVAL, EPERM, ESRCH. So if we want to guarantee caller no need to store a temporary errno in this curcirmstance, we could just rewrite virProcessKill when signum == 0, do not overwrite errno when failure.
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
In lxcContainerStart, when user namespace is not supported, the virReportSystemError is called. But the first argument should be ENOTSUPP, instead of VIR_ERR_CONFIG_UNSUPPORTED. Signed-off-by: Jincheng Miao <jmiao@redhat.com> --- src/lxc/lxc_container.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index 4d89677..343df47 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -2020,7 +2020,7 @@ int lxcContainerStart(virDomainDefPtr def, VIR_DEBUG("Enable user namespace"); cflags |= CLONE_NEWUSER; } else { - virReportSystemError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + virReportSystemError(ENOTSUP, "%s", _("Kernel doesn't support user namespace")); VIR_FREE(stack); return -1; -- 1.8.3.1
On Tue, Jul 15, 2014 at 04:32:01PM +0800, Jincheng Miao wrote:
In lxcContainerStart, when user namespace is not supported, the virReportSystemError is called. But the first argument should be ENOTSUPP, instead of VIR_ERR_CONFIG_UNSUPPORTED.
Signed-off-by: Jincheng Miao <jmiao@redhat.com> --- src/lxc/lxc_container.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index 4d89677..343df47 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -2020,7 +2020,7 @@ int lxcContainerStart(virDomainDefPtr def, VIR_DEBUG("Enable user namespace"); cflags |= CLONE_NEWUSER; } else { - virReportSystemError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + virReportSystemError(ENOTSUP, "%s", _("Kernel doesn't support user namespace")); VIR_FREE(stack); return -1; -- 1.8.3.1
Using virReportSystemError() with anything else than 'errno' is a misuse by my standards. Just use virReportError() instead if there's VIR_ERR_CONFIG_UNSUPPORTED used. Martin P.S.: The same applies for following patches.
In openvzReadFSConf, when barrier:limit are so long to overflow, pass EOVERFLOW to virReportSystemError, instead of VIR_ERR_OVERFLOW. Signed-off-by: Jincheng Miao <jmiao@redhat.com> --- src/openvz/openvz_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c index 856c9f5..8fdd4e9 100644 --- a/src/openvz/openvz_conf.c +++ b/src/openvz/openvz_conf.c @@ -405,7 +405,7 @@ openvzReadFSConf(virDomainDefPtr def, /* Ensure that we can multiply by 1024 without overflowing. */ if (barrier > ULLONG_MAX / 1024 || limit > ULLONG_MAX / 1024) { - virReportSystemError(VIR_ERR_OVERFLOW, "%s", + virReportSystemError(EOVERFLOW, "%s", _("Unable to parse quota")); goto error; } -- 1.8.3.1
In virObjectLockableNew, when virMutexInit fails, virReportSystemError should use errno to get the right error number, instead of VIR_ERR_INTERNAL_ERROR. Signed-off-by: Jincheng Miao <jmiao@redhat.com> --- src/util/virobject.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/virobject.c b/src/util/virobject.c index 6cb84b4..b5d2c02 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -220,7 +220,7 @@ void *virObjectLockableNew(virClassPtr klass) return NULL; if (virMutexInit(&obj->lock) < 0) { - virReportSystemError(VIR_ERR_INTERNAL_ERROR, "%s", + virReportSystemError(errno, "%s", _("Unable to initialize mutex")); virObjectUnref(obj); return NULL; -- 1.8.3.1
On Tue, Jul 15, 2014 at 04:32:03PM +0800, Jincheng Miao wrote:
In virObjectLockableNew, when virMutexInit fails, virReportSystemError should use errno to get the right error number, instead of VIR_ERR_INTERNAL_ERROR.
Signed-off-by: Jincheng Miao <jmiao@redhat.com> --- src/util/virobject.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/virobject.c b/src/util/virobject.c index 6cb84b4..b5d2c02 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -220,7 +220,7 @@ void *virObjectLockableNew(virClassPtr klass) return NULL;
if (virMutexInit(&obj->lock) < 0) { - virReportSystemError(VIR_ERR_INTERNAL_ERROR, "%s", + virReportSystemError(errno, "%s", _("Unable to initialize mutex"));
I'm not sure errno is set when using our virMutexInit(). Most of the code uses virReportError instead, I suggest using that. This should be changed everywhere in the code. Rough idea of the places could be gotten by the following command: git grep -nA5 virMutexInit | grep SystemError but as I said, only rough idea :) Martin
----- Original Message -----
I'm not sure errno is set when using our virMutexInit(). Most of the code uses virReportError instead, I suggest using that. This should
Actually, the implement of virMutexInit() already set errno when failure: int virMutexInit(virMutexPtr m) { int ret; pthread_mutexattr_t attr; pthread_mutexattr_init(&attr); pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_NORMAL); ret = pthread_mutex_init(&m->lock, &attr); pthread_mutexattr_destroy(&attr); if (ret != 0) { errno = ret; return -1; } return 0; }
be changed everywhere in the code. Rough idea of the places could be
I think it worthy of adding after all virMutexInit, I will include it in my V2 patchset.
gotten by the following command:
git grep -nA5 virMutexInit | grep SystemError
but as I said, only rough idea :)
Martin
On Tue, Jul 15, 2014 at 10:47:04PM -0400, Jincheng Miao wrote:
----- Original Message -----
I'm not sure errno is set when using our virMutexInit(). Most of the code uses virReportError instead, I suggest using that. This should
Actually, the implement of virMutexInit() already set errno when failure:
int virMutexInit(virMutexPtr m) { int ret; pthread_mutexattr_t attr; pthread_mutexattr_init(&attr); pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_NORMAL); ret = pthread_mutex_init(&m->lock, &attr); pthread_mutexattr_destroy(&attr); if (ret != 0) { errno = ret; return -1; } return 0; }
Oh, sorry; /me facepalms... You're right then, but if you want to change it everywhere in the code (which I don't require), it would be better to create a virMutexInitInternal that takes 2 parameters; the second being a bool that controls error reporting and virMutexInit and virMutexInitQuiet would be two macros. Similarly to virVasprintf for example.
be changed everywhere in the code. Rough idea of the places could be
I think it worthy of adding after all virMutexInit, I will include it in my V2 patchset.
gotten by the following command:
git grep -nA5 virMutexInit | grep SystemError
but as I said, only rough idea :)
Martin
----- Original Message -----
Oh, sorry; /me facepalms...
You're right then, but if you want to change it everywhere in the code (which I don't require), it would be better to create a virMutexInitInternal that takes 2 parameters; the second being a bool that controls error reporting and virMutexInit and virMutexInitQuiet would be two macros. Similarly to virVasprintf for example.
Good idea :)
----- Original Message -----
----- Original Message -----
Oh, sorry; /me facepalms...
You're right then, but if you want to change it everywhere in the code (which I don't require), it would be better to create a virMutexInitInternal that takes 2 parameters; the second being a bool that controls error reporting and virMutexInit and virMutexInitQuiet would be two macros. Similarly to virVasprintf for example.
Good idea :)
I will remove this patch from this patchset on V2, and send a separate one for virMutexInit* implement.
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (3)
-
Eric Blake -
Jincheng Miao -
Martin Kletzander