3:31 a.m.
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
3:32 a.m.
New subject: [libvirt] [PATCH 1/4] qemu: fix wrong errno report in qemuMonitorOpenUnix
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(a)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
8:28 a.m.
New subject: [libvirt] [PATCH 1/4] qemu: fix wrong errno report in qemuMonitorOpenUnix
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(a)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
10:24 p.m.
New subject: [libvirt] [PATCH 1/4] qemu: fix wrong errno report in qemuMonitorOpenUnix
----- 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
3:32 a.m.
New subject: [libvirt] [PATCH 2/4] lxc: print ENOTSUP when usernamespace is not supported
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(a)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
4:24 a.m.
New subject: [libvirt] [PATCH 2/4] lxc: print ENOTSUP when usernamespace is not supported
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(a)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.
3:32 a.m.
New subject: [libvirt] [PATCH 3/4] openvz: print EOVERFLOW when barrier:limit are too long
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(a)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
3:32 a.m.
New subject: [libvirt] [PATCH 4/4] util: print errno in virObjectLockableNew
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(a)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
4:22 a.m.
New subject: [libvirt] [PATCH 4/4] util: print errno in virObjectLockableNew
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(a)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
9:47 p.m.
New subject: [libvirt] [PATCH 4/4] util: print errno in virObjectLockableNew
----- 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
1:44 a.m.
New subject: [libvirt] [PATCH 4/4] util: print errno in virObjectLockableNew
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
>
1:55 a.m.
New subject: [libvirt] [PATCH 4/4] util: print errno in virObjectLockableNew
----- 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 :)
2:56 a.m.
New subject: [libvirt] [PATCH 4/4] util: print errno in virObjectLockableNew
----- 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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
3782
days inactive
3783
days old
12 comments
3 participants
participants (3)
-
Eric Blake -
Jincheng Miao -
Martin Kletzander