April 2015 - Devel - libvirt List Archives

[libvirt] [PATCH] Fix virCgroupGetPercpuStats with non-continuous present CPUs
by Ján Tomko 15 Apr '15

15 Apr '15

Per-cpu stats are only shown for present CPUs in the cgroups, but we were only parsing the largest CPU number from /sys/devices/system/cpu/present and looking for stats even for non-present CPUs. This resulted in: internal error: cpuacct parse error --- cfg.mk | 2 +- src/nodeinfo.c | 17 +++++++++++++++++ src/nodeinfo.h | 1 + src/util/vircgroup.c | 24 ++++++++++++++++++------ tests/vircgroupmock.c | 44 ++++++++++++++++++++++++++++++++++++++------ tests/vircgrouptest.c | 47 +++++++++++++++++++++++++++++++++++------------ 6 files changed, 110 insertions(+), 25 deletions(-) diff --git a/cfg.mk b/cfg.mk index 21f83c3..70612f8 100644 --- a/cfg.mk +++ b/cfg.mk @@ -1095,7 +1095,7 @@ exclude_file_name_regexp--sc_prohibit_asprintf = \ ^(bootstrap.conf$$|src/util/virstring\.[ch]$$|tests/vircgroupmock\.c$$) exclude_file_name_regexp--sc_prohibit_strdup = \ - ^(docs/|examples/|src/util/virstring\.c|tests/virnetserverclientmock.c$$) + ^(docs/|examples/|src/util/virstring\.c|tests/vir(netserverclient|cgroup)mock.c$$) exclude_file_name_regexp--sc_prohibit_close = \ (\.p[yl]$$|\.spec\.in$$|^docs/|^(src/util/virfile\.c|src/libvirt-stream\.c|tests/vir(cgroup|pci)mock\.c)$$) diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 3c22ebc..3a27c22 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -1242,6 +1242,23 @@ nodeGetCPUCount(void) } virBitmapPtr +nodeGetPresentCPUBitmap(void) +{ + int max_present; + + if ((max_present = nodeGetCPUCount()) < 0) + return NULL; + +#ifdef __linux__ + if (virFileExists(SYSFS_SYSTEM_PATH "/cpu/present")) + return linuxParseCPUmap(max_present, SYSFS_SYSTEM_PATH "/cpu/present"); +#endif + virReportError(VIR_ERR_NO_SUPPORT, "%s", + _("non-continuous host cpu numbers not implemented on this platform")); + return NULL; +} + +virBitmapPtr nodeGetCPUBitmap(int *max_id ATTRIBUTE_UNUSED) { #ifdef __linux__ diff --git a/src/nodeinfo.h b/src/nodeinfo.h index a993c63..047bd5c 100644 --- a/src/nodeinfo.h +++ b/src/nodeinfo.h @@ -43,6 +43,7 @@ int nodeGetCellsFreeMemory(unsigned long long *freeMems, int nodeGetMemory(unsigned long long *mem, unsigned long long *freeMem); +virBitmapPtr nodeGetPresentCPUBitmap(void); virBitmapPtr nodeGetCPUBitmap(int *max_id); int nodeGetCPUCount(void); diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index fe34290..e65617a 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -2985,7 +2985,8 @@ static int virCgroupGetPercpuVcpuSum(virCgroupPtr group, unsigned int nvcpupids, unsigned long long *sum_cpu_time, - unsigned int num) + size_t nsum, + virBitmapPtr cpumap) { int ret = -1; size_t i; @@ -2995,7 +2996,7 @@ virCgroupGetPercpuVcpuSum(virCgroupPtr group, for (i = 0; i < nvcpupids; i++) { char *pos; unsigned long long tmp; - size_t j; + ssize_t j; if (virCgroupNewVcpu(group, i, false, &group_vcpu) < 0) goto cleanup; @@ -3004,7 +3005,9 @@ virCgroupGetPercpuVcpuSum(virCgroupPtr group, goto cleanup; pos = buf; - for (j = 0; j < num; j++) { + for (j = virBitmapNextSetBit(cpumap, -1); + j >= 0 && j < nsum; + j = virBitmapNextSetBit(cpumap, j)) { if (virStrToLong_ull(pos, &pos, 10, &tmp) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cpuacct parse error")); @@ -3042,6 +3045,7 @@ virCgroupGetPercpuStats(virCgroupPtr group, virTypedParameterPtr ent; int param_idx; unsigned long long cpu_time; + virBitmapPtr cpumap = NULL; /* return the number of supported params */ if (nparams == 0 && ncpus != 0) { @@ -3052,9 +3056,11 @@ virCgroupGetPercpuStats(virCgroupPtr group, } /* To parse account file, we need to know how many cpus are present. */ - if ((total_cpus = nodeGetCPUCount()) < 0) + if (!(cpumap = nodeGetPresentCPUBitmap())) return rv; + total_cpus = virBitmapSize(cpumap); + if (ncpus == 0) return total_cpus; @@ -3077,7 +3083,11 @@ virCgroupGetPercpuStats(virCgroupPtr group, need_cpus = MIN(total_cpus, start_cpu + ncpus); for (i = 0; i < need_cpus; i++) { - if (virStrToLong_ull(pos, &pos, 10, &cpu_time) < 0) { + bool present; + ignore_value(virBitmapGetBit(cpumap, i, &present)); + if (!present) { + cpu_time = 0; + } else if (virStrToLong_ull(pos, &pos, 10, &cpu_time) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cpuacct parse error")); goto cleanup; @@ -3097,7 +3107,8 @@ virCgroupGetPercpuStats(virCgroupPtr group, if (VIR_ALLOC_N(sum_cpu_time, need_cpus) < 0) goto cleanup; - if (virCgroupGetPercpuVcpuSum(group, nvcpupids, sum_cpu_time, need_cpus) < 0) + if (virCgroupGetPercpuVcpuSum(group, nvcpupids, sum_cpu_time, need_cpus, + cpumap) < 0) goto cleanup; for (i = start_cpu; i < need_cpus; i++) { @@ -3113,6 +3124,7 @@ virCgroupGetPercpuStats(virCgroupPtr group, rv = param_idx + 1; cleanup: + virBitmapFree(cpumap); VIR_FREE(sum_cpu_time); VIR_FREE(buf); return rv; diff --git a/tests/vircgroupmock.c b/tests/vircgroupmock.c index 6e0a0e7..f2fee7d 100644 --- a/tests/vircgroupmock.c +++ b/tests/vircgroupmock.c @@ -51,6 +51,8 @@ const char *fakedevicedir1 = FAKEDEVDIR1; # define SYSFS_PREFIX "/not/really/sys/fs/cgroup/" +# define SYSFS_CPU_PRESENT "/sys/devices/system/cpu/present" +# define SYSFS_CPU_PRESENT_MOCKED "devices_system_cpu_present" /* * The plan: @@ -215,7 +217,15 @@ static int make_controller(const char *path, mode_t mode) "user 216687025\n" "system 43421396\n"); MAKE_FILE("cpuacct.usage", "2787788855799582\n"); - MAKE_FILE("cpuacct.usage_percpu", "1413142688153030 1374646168910542\n"); + MAKE_FILE("cpuacct.usage_percpu", + "7059492996 0 0 0 0 0 0 0 4180532496 0 0 0 0 0 0 0 " + "1957541268 0 0 0 0 0 0 0 2065932204 0 0 0 0 0 0 0 " + "18228689414 0 0 0 0 0 0 0 4245525148 0 0 0 0 0 0 0 " + "2911161568 0 0 0 0 0 0 0 1407758136 0 0 0 0 0 0 0 " + "1836807700 0 0 0 0 0 0 0 1065296618 0 0 0 0 0 0 0 " + "2046213266 0 0 0 0 0 0 0 747889778 0 0 0 0 0 0 0 " + "709566900 0 0 0 0 0 0 0 444777342 0 0 0 0 0 0 0 " + "5683512916 0 0 0 0 0 0 0 635751356 0 0 0 0 0 0 0\n"); } else if (STRPREFIX(controller, "cpuset")) { MAKE_FILE("cpuset.cpu_exclusive", "1\n"); if (STREQ(controller, "cpuset")) @@ -431,6 +441,9 @@ static void init_sysfs(void) MAKE_CONTROLLER("blkio"); MAKE_CONTROLLER("memory"); MAKE_CONTROLLER("freezer"); + + if (make_file(fakesysfsdir, SYSFS_CPU_PRESENT_MOCKED, "8-23,48-159\n") < 0) + abort(); } @@ -623,21 +636,27 @@ int __xstat(int ver, const char *path, struct stat *sb) int stat(const char *path, struct stat *sb) { + char *newpath = NULL; int ret; init_syms(); - if (STRPREFIX(path, SYSFS_PREFIX)) { + if (STREQ(path, SYSFS_CPU_PRESENT)) { + init_sysfs(); + if (asprintf(&newpath, "%s/%s", + fakesysfsdir, + SYSFS_CPU_PRESENT_MOCKED) < 0) { + errno = ENOMEM; + return -1; + } + } else if (STRPREFIX(path, SYSFS_PREFIX)) { init_sysfs(); - char *newpath; if (asprintf(&newpath, "%s/%s", fakesysfsdir, path + strlen(SYSFS_PREFIX)) < 0) { errno = ENOMEM; return -1; } - ret = realstat(newpath, sb); - free(newpath); } else if (STRPREFIX(path, fakedevicedir0)) { sb->st_mode = S_IFBLK; sb->st_rdev = makedev(8, 0); @@ -647,8 +666,11 @@ int stat(const char *path, struct stat *sb) sb->st_rdev = makedev(9, 0); return 0; } else { - ret = realstat(path, sb); + if (!(newpath = strdup(path))) + return -1; } + ret = realstat(newpath, sb); + free(newpath); return ret; } @@ -682,6 +704,16 @@ int open(const char *path, int flags, ...) init_syms(); + if (STREQ(path, SYSFS_CPU_PRESENT)) { + init_sysfs(); + if (asprintf(&newpath, "%s/%s", + fakesysfsdir, + SYSFS_CPU_PRESENT_MOCKED) < 0) { + errno = ENOMEM; + return -1; + } + } + if (STRPREFIX(path, SYSFS_PREFIX)) { init_sysfs(); if (asprintf(&newpath, "%s/%s", diff --git a/tests/vircgrouptest.c b/tests/vircgrouptest.c index 35ac0c0..b65ea3f 100644 --- a/tests/vircgrouptest.c +++ b/tests/vircgrouptest.c @@ -538,12 +538,35 @@ static int testCgroupGetPercpuStats(const void *args ATTRIBUTE_UNUSED) virCgroupPtr cgroup = NULL; size_t i; int rv, ret = -1; - virTypedParameter params[2]; + virTypedParameterPtr params = NULL; +# define EXPECTED_NCPUS 160 - // TODO: mock nodeGetCPUCount() as well & check 2nd cpu, too unsigned long long expected[] = { - 1413142688153030ULL + 0, 0, 0, 0, 0, 0, 0, 0, + 7059492996, 0, 0, 0, 0, 0, 0, 0, + 4180532496, 0, 0, 0, 0, 0, 0, 0, + 0, 0, 0, 0, 0, 0, 0, 0, + 0, 0, 0, 0, 0, 0, 0, 0, + 0, 0, 0, 0, 0, 0, 0, 0, + 1957541268, 0, 0, 0, 0, 0, 0, 0, + 2065932204, 0, 0, 0, 0, 0, 0, 0, + 18228689414, 0, 0, 0, 0, 0, 0, 0, + 4245525148, 0, 0, 0, 0, 0, 0, 0, + 2911161568, 0, 0, 0, 0, 0, 0, 0, + 1407758136, 0, 0, 0, 0, 0, 0, 0, + 1836807700, 0, 0, 0, 0, 0, 0, 0, + 1065296618, 0, 0, 0, 0, 0, 0, 0, + 2046213266, 0, 0, 0, 0, 0, 0, 0, + 747889778, 0, 0, 0, 0, 0, 0, 0, + 709566900, 0, 0, 0, 0, 0, 0, 0, + 444777342, 0, 0, 0, 0, 0, 0, 0, + 5683512916, 0, 0, 0, 0, 0, 0, 0, + 635751356, 0, 0, 0, 0, 0, 0, 0, }; + verify(ARRAY_CARDINALITY(expected) == EXPECTED_NCPUS); + + if (VIR_ALLOC_N(params, EXPECTED_NCPUS) < 0) + goto cleanup; if ((rv = virCgroupNewPartition("/virtualmachines", true, (1 << VIR_CGROUP_CONTROLLER_CPU) | @@ -553,37 +576,37 @@ static int testCgroupGetPercpuStats(const void *args ATTRIBUTE_UNUSED) goto cleanup; } - if (nodeGetCPUCount() < 1) { + if (nodeGetCPUCount() != EXPECTED_NCPUS) { fprintf(stderr, "Unexpected: nodeGetCPUCount() yields: %d\n", nodeGetCPUCount()); goto cleanup; } if ((rv = virCgroupGetPercpuStats(cgroup, params, - 2, 0, 1, 0)) < 0) { + 1, 0, EXPECTED_NCPUS, 0)) < 0) { fprintf(stderr, "Failed call to virCgroupGetPercpuStats for /virtualmachines cgroup: %d\n", -rv); goto cleanup; } - for (i = 0; i < ARRAY_CARDINALITY(expected); i++) { + for (i = 0; i < EXPECTED_NCPUS; i++) { if (!STREQ(params[i].field, VIR_DOMAIN_CPU_STATS_CPUTIME)) { fprintf(stderr, - "Wrong parameter name value from virCgroupGetPercpuStats (is: %s)\n", - params[i].field); + "Wrong parameter name value from virCgroupGetPercpuStats at %zu (is: %s)\n", + i, params[i].field); goto cleanup; } if (params[i].type != VIR_TYPED_PARAM_ULLONG) { fprintf(stderr, - "Wrong parameter value type from virCgroupGetPercpuStats (is: %d)\n", - params[i].type); + "Wrong parameter value type from virCgroupGetPercpuStats at %zu (is: %d)\n", + i, params[i].type); goto cleanup; } if (params[i].value.ul != expected[i]) { fprintf(stderr, - "Wrong value from virCgroupGetMemoryUsage (expected %llu)\n", - params[i].value.ul); + "Wrong value from virCgroupGetMemoryUsage at %zu (expected %llu)\n", + i, params[i].value.ul); goto cleanup; } } -- 2.0.4

3 4

[libvirt] [PATCH] tests: fix build on old 32-bit platforms
by Eric Blake 15 Apr '15

15 Apr '15

gcc 4.1.2 (hello RHEL 5) on 32-bit platforms complains: vircgrouptest.c: In function 'testCgroupGetPercpuStats': vircgrouptest.c:627: warning: integer constant is too large for 'long' type vircgrouptest.c:628: warning: this decimal constant is unsigned only in ISO C90 vircgrouptest.c:634: warning: integer constant is too large for 'long' type vircgrouptest.c:635: warning: this decimal constant is unsigned only in ISO C90 vircgrouptest.c:636: warning: this decimal constant is unsigned only in ISO C90 vircgrouptest.c:644: warning: integer constant is too large for 'long' type * tests/vircgrouptest.c (testCgroupGetPercpuStats): Use ULL suffix. Signed-off-by: Eric Blake <eblake(a)redhat.com> --- Pushing under the build-breaker rule tests/vircgrouptest.c | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/tests/vircgrouptest.c b/tests/vircgrouptest.c index 2b30258..ad49425 100644 --- a/tests/vircgrouptest.c +++ b/tests/vircgrouptest.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2013-2014 Red Hat, Inc. + * Copyright (C) 2013-2015 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -624,25 +624,25 @@ static int testCgroupGetPercpuStats(const void *args ATTRIBUTE_UNUSED) unsigned long long expected[EXPECTED_NCPUS] = { 0, 0, 0, 0, 0, 0, 0, 0, - 7059492996, 0, 0, 0, 0, 0, 0, 0, - 4180532496, 0, 0, 0, 0, 0, 0, 0, + 7059492996ULL, 0, 0, 0, 0, 0, 0, 0, + 4180532496ULL, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, - 1957541268, 0, 0, 0, 0, 0, 0, 0, - 2065932204, 0, 0, 0, 0, 0, 0, 0, - 18228689414, 0, 0, 0, 0, 0, 0, 0, - 4245525148, 0, 0, 0, 0, 0, 0, 0, - 2911161568, 0, 0, 0, 0, 0, 0, 0, - 1407758136, 0, 0, 0, 0, 0, 0, 0, - 1836807700, 0, 0, 0, 0, 0, 0, 0, - 1065296618, 0, 0, 0, 0, 0, 0, 0, - 2046213266, 0, 0, 0, 0, 0, 0, 0, - 747889778, 0, 0, 0, 0, 0, 0, 0, - 709566900, 0, 0, 0, 0, 0, 0, 0, - 444777342, 0, 0, 0, 0, 0, 0, 0, - 5683512916, 0, 0, 0, 0, 0, 0, 0, - 635751356, 0, 0, 0, 0, 0, 0, 0, + 1957541268ULL, 0, 0, 0, 0, 0, 0, 0, + 2065932204ULL, 0, 0, 0, 0, 0, 0, 0, + 18228689414ULL, 0, 0, 0, 0, 0, 0, 0, + 4245525148ULL, 0, 0, 0, 0, 0, 0, 0, + 2911161568ULL, 0, 0, 0, 0, 0, 0, 0, + 1407758136ULL, 0, 0, 0, 0, 0, 0, 0, + 1836807700ULL, 0, 0, 0, 0, 0, 0, 0, + 1065296618ULL, 0, 0, 0, 0, 0, 0, 0, + 2046213266ULL, 0, 0, 0, 0, 0, 0, 0, + 747889778ULL, 0, 0, 0, 0, 0, 0, 0, + 709566900ULL, 0, 0, 0, 0, 0, 0, 0, + 444777342ULL, 0, 0, 0, 0, 0, 0, 0, + 5683512916ULL, 0, 0, 0, 0, 0, 0, 0, + 635751356ULL, 0, 0, 0, 0, 0, 0, 0, }; if (VIR_ALLOC_N(params, EXPECTED_NCPUS) < 0) -- 2.1.0

1 0

[libvirt] [PATCH] virNetSocketNewConnectUNIX: Use flocks when spawning a daemon
by Michal Privoznik 15 Apr '15

15 Apr '15

https://bugzilla.redhat.com/show_bug.cgi?id=1200149 Even though we have a mutex mechanism so that two clients don't spawn two daemons, it's not strong enough. It can happen that while one client is spawning the daemon, the other one fails to connect. Basically two possible errors can happen: error: Failed to connect socket to '/home/mprivozn/.cache/libvirt/libvirt-sock': Connection refused or: error: Failed to connect socket to '/home/mprivozn/.cache/libvirt/libvirt-sock': No such file or directory The problem in both cases is, the daemon is only starting up, while we are trying to connect (and fail). We should postpone the connecting phase until the daemon is started (by the other thread that is spawning it). In order to do that, create a file lock 'libvirt-lock' in the directory where session daemon would create its socket. So even when called from multiple processes, spawning a daemon will serialize on the file lock. So only the first to come will spawn the daemon. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com> --- diff to v1: -hopefully this one is race free src/rpc/virnetsocket.c | 148 ++++++++++++------------------------------------- 1 file changed, 36 insertions(+), 112 deletions(-) diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index 6b019cc..2b891ae 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -123,7 +123,7 @@ VIR_ONCE_GLOBAL_INIT(virNetSocket) #ifndef WIN32 -static int virNetSocketForkDaemon(const char *binary, int passfd) +static int virNetSocketForkDaemon(const char *binary) { int ret; virCommandPtr cmd = virCommandNewArgList(binary, @@ -136,10 +136,6 @@ static int virNetSocketForkDaemon(const char *binary, int passfd) virCommandAddEnvPassBlockSUID(cmd, "XDG_RUNTIME_DIR", NULL); virCommandClearCaps(cmd); virCommandDaemonize(cmd); - if (passfd) { - virCommandPassFD(cmd, passfd, VIR_COMMAND_PASS_FD_CLOSE_PARENT); - virCommandPassListenFDs(cmd); - } ret = virCommandRun(cmd, NULL); virCommandFree(cmd); return ret; @@ -543,10 +539,10 @@ int virNetSocketNewConnectUNIX(const char *path, const char *binary, virNetSocketPtr *retsock) { - char *binname = NULL; - char *pidpath = NULL; - int fd, passfd = -1; - int pidfd = -1; + char *lockpath = NULL; + int lockfd = -1; + int fd = -1; + int retries = 100; virSocketAddr localAddr; virSocketAddr remoteAddr; @@ -561,6 +557,22 @@ int virNetSocketNewConnectUNIX(const char *path, return -1; } + if (spawnDaemon) { + if (virPidFileConstructPath(false, NULL, "libvirt-lock", &lockpath) < 0) + goto error; + + if ((lockfd = open(lockpath, O_RDWR | O_CREAT, 0600)) < 0 || + virSetCloseExec(lockfd) < 0) { + virReportSystemError(errno, _("Unable to create lock '%s'"), lockpath); + goto error; + } + + if (virFileLock(lockfd, false, 0, 1, true) < 0) { + virReportSystemError(errno, _("Unable to lock '%s'"), lockpath); + goto error; + } + } + if ((fd = socket(PF_UNIX, SOCK_STREAM, 0)) < 0) { virReportSystemError(errno, "%s", _("Failed to create socket")); goto error; @@ -574,108 +586,25 @@ int virNetSocketNewConnectUNIX(const char *path, if (remoteAddr.data.un.sun_path[0] == '@') remoteAddr.data.un.sun_path[0] = '\0'; - retry: - if (connect(fd, &remoteAddr.data.sa, remoteAddr.len) < 0) { - int status = 0; - pid_t pid = 0; - - if (!spawnDaemon) { + while (retries && + connect(fd, &remoteAddr.data.sa, remoteAddr.len) < 0) { + if (!(spawnDaemon && errno == ENOENT)) { virReportSystemError(errno, _("Failed to connect socket to '%s'"), path); goto error; } - if (!(binname = last_component(binary)) || binname[0] == '\0') { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Cannot determine basename for binary '%s'"), - binary); - goto error; - } - - if (virPidFileConstructPath(false, NULL, binname, &pidpath) < 0) - goto error; - - pidfd = virPidFileAcquirePath(pidpath, false, getpid()); - if (pidfd < 0) { - /* - * This can happen in a very rare case of two clients spawning two - * daemons at the same time, and the error in the logs that gets - * reset here can be a clue to some future debugging. - */ - virResetLastError(); - spawnDaemon = false; - goto retry; - } - - if ((passfd = socket(PF_UNIX, SOCK_STREAM, 0)) < 0) { - virReportSystemError(errno, "%s", _("Failed to create socket")); + if (virNetSocketForkDaemon(binary) < 0) goto error; - } - /* - * We already even acquired the pidfile, so no one else should be using - * @path right now. So we're OK to unlink it and paying attention to - * the return value makes no real sense here. Only if it's not an - * abstract socket, of course. - */ - if (path[0] != '@') - unlink(path); - - /* - * We have to fork() here, because umask() is set per-process, chmod() - * is racy and fchmod() has undefined behaviour on sockets according to - * POSIX, so it doesn't work outside Linux. - */ - if ((pid = virFork()) < 0) - goto error; - - if (pid == 0) { - umask(0077); - if (bind(passfd, &remoteAddr.data.sa, remoteAddr.len) < 0) - _exit(EXIT_FAILURE); - - _exit(EXIT_SUCCESS); - } + retries--; + usleep(5000); + } - if (virProcessWait(pid, &status, false) < 0) - goto error; - - if (status != EXIT_SUCCESS) { - /* - * OK, so the child failed to bind() the socket. This may mean that - * another daemon was starting at the same time and succeeded with - * its bind() (even though it should not happen because we using a - * pidfile for the race). So we'll try connecting again, but this - * time without spawning the daemon. - */ - spawnDaemon = false; - virPidFileDeletePath(pidpath); - VIR_FORCE_CLOSE(pidfd); - VIR_FORCE_CLOSE(passfd); - goto retry; - } - - if (listen(passfd, 0) < 0) { - virReportSystemError(errno, "%s", - _("Failed to listen on socket that's about " - "to be passed to the daemon")); - goto error; - } - - if (connect(fd, &remoteAddr.data.sa, remoteAddr.len) < 0) { - virReportSystemError(errno, _("Failed to connect socket to '%s'"), - path); - goto error; - } - - /* - * Do we need to eliminate the super-rare race here any more? It would - * need incorporating the following VIR_FORCE_CLOSE() into a - * virCommandHook inside a virNetSocketForkDaemon(). - */ - VIR_FORCE_CLOSE(pidfd); - if (virNetSocketForkDaemon(binary, passfd) < 0) - goto error; + if (lockfd) { + unlink(lockpath); + VIR_FORCE_CLOSE(lockfd); + VIR_FREE(lockpath); } localAddr.len = sizeof(localAddr.data); @@ -687,19 +616,14 @@ int virNetSocketNewConnectUNIX(const char *path, if (!(*retsock = virNetSocketNew(&localAddr, &remoteAddr, true, fd, -1, 0))) goto error; - VIR_FREE(pidpath); - return 0; error: - if (pidfd >= 0) - virPidFileDeletePath(pidpath); - VIR_FREE(pidpath); + if (lockfd) + unlink(lockpath); + VIR_FREE(lockpath); VIR_FORCE_CLOSE(fd); - VIR_FORCE_CLOSE(passfd); - VIR_FORCE_CLOSE(pidfd); - if (spawnDaemon) - unlink(path); + VIR_FORCE_CLOSE(lockfd); return -1; } #else -- 2.0.5

4 3

[libvirt] [PATCHv2 0/5] Strip control codes from virBufferEscapeString
by Ján Tomko 15 Apr '15

15 Apr '15

For https://bugzilla.redhat.com/show_bug.cgi?id=1184131 https://bugzilla.redhat.com/show_bug.cgi?id=1066564 v1: https://www.redhat.com/archives/libvir-list/2015-March/msg01529.html v2: * added patches 1-4 that should prevent us from calling virBufferEscapeString with control codes * patch 5 is unchanged Ján Tomko (5): tests: rename testStripIPv6BracketsData to testStripData Add functions dealing with control characters in strings Strip control characters from sysfs attributes Ignore storage volumes with control codes in their names Strip control codes in virBufferEscapeString src/libvirt_private.syms | 2 ++ src/node_device/node_device_udev.c | 2 ++ src/storage/storage_backend_fs.c | 6 +++++ src/util/virbuffer.c | 14 ++++++++--- src/util/virstring.c | 39 ++++++++++++++++++++++++++++++ src/util/virstring.h | 2 ++ tests/virbuftest.c | 49 ++++++++++++++++++++++++++++++++++++++ tests/virstringtest.c | 45 +++++++++++++++++++++++++++++++--- 8 files changed, 153 insertions(+), 6 deletions(-) -- 2.0.5

2 11

[libvirt] [PATCH 0/2] Couple of virnetdevtest cleanups
by Michal Privoznik 15 Apr '15

15 Apr '15

*** BLURB HERE *** Michal Privoznik (2): tests: Add virnetdevtestdata to EXTRA_DIST Cleanup "/sys/class/net" usage src/Makefile.am | 4 ++-- src/parallels/parallels_network.c | 3 +-- src/util/virnetdev.c | 5 ++--- src/util/virnetdev.h | 2 ++ src/util/virnetdevbridge.c | 1 - src/util/virnetdevmacvlan.c | 28 ++++++++++++++-------------- tests/Makefile.am | 1 + 7 files changed, 22 insertions(+), 22 deletions(-) -- 2.0.5

2 5

[libvirt] [PATCH v2] Link libvirt_util with datatypes
by Martin Kletzander 15 Apr '15

15 Apr '15

Most of the types in datatypes.[hc] depend on virObject and virClass, but they were specified separatedly from that. We were lucky enough for this to work because wherever the datatypes files were used, that file (binary/shared object) was linked to libvirt_util as well. Fixing this comes up as a pretty nice cleanup. Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com> --- src/Makefile.am | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/src/Makefile.am b/src/Makefile.am index 91a4c17..84f489f 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -1,6 +1,6 @@ ## Process this file with automake to produce Makefile.in -## Copyright (C) 2005-2014 Red Hat, Inc. +## Copyright (C) 2005-2015 Red Hat, Inc. ## ## This library is free software; you can redistribute it and/or ## modify it under the terms of the GNU Lesser General Public @@ -86,9 +86,12 @@ augeas_DATA = augeastestdir = $(datadir)/augeas/lenses/tests augeastest_DATA = +DATATYPES_SOURCES = datatypes.h datatypes.c + # These files are not related to driver APIs. Simply generic # helper APIs for various purposes UTIL_SOURCES = \ + $(DATATYPES_SOURCES) \ util/viralloc.c util/viralloc.h \ util/virarch.h util/virarch.c \ util/viratomic.h util/viratomic.c \ @@ -185,7 +188,6 @@ util/virkeymaps.h: $(srcdir)/util/keymaps.csv \ # Internal generic driver infrastructure NODE_INFO_SOURCES = nodeinfo.h nodeinfo.c nodeinfopriv.h -DATATYPES_SOURCES = datatypes.h datatypes.c DRIVER_SOURCES = \ driver.c driver.h \ driver-hypervisor.h \ @@ -198,7 +200,6 @@ DRIVER_SOURCES = \ driver-storage.h \ driver-stream.h \ internal.h \ - $(DATATYPES_SOURCES) \ fdstream.c fdstream.h \ $(NODE_INFO_SOURCES) \ libvirt.c libvirt_internal.h \ @@ -671,10 +672,6 @@ LXC_CONTROLLER_SOURCES = \ lxc/lxc_fuse.c lxc/lxc_fuse.h \ lxc/lxc_controller.c -SECURITY_DRIVER_APPARMOR_HELPER_SOURCES = \ - $(DATATYPES_SOURCES) \ - security/virt-aa-helper.c - PHYP_DRIVER_SOURCES = \ phyp/phyp_driver.c phyp/phyp_driver.h @@ -2604,8 +2601,7 @@ libexec_PROGRAMS += libvirt_lxc libvirt_lxc_SOURCES = \ $(LXC_CONTROLLER_SOURCES) \ - $(NODE_INFO_SOURCES) \ - $(DATATYPES_SOURCES) + $(NODE_INFO_SOURCES) libvirt_lxc_LDFLAGS = \ $(AM_LDFLAGS) \ $(PIE_LDFLAGS) \ @@ -2643,7 +2639,7 @@ if WITH_SECDRIVER_APPARMOR if WITH_LIBVIRTD libexec_PROGRAMS += virt-aa-helper -virt_aa_helper_SOURCES = $(SECURITY_DRIVER_APPARMOR_HELPER_SOURCES) +virt_aa_helper_SOURCES = security/virt-aa-helper.c virt_aa_helper_LDFLAGS = \ $(AM_LDFLAGS) \ @@ -2666,7 +2662,7 @@ virt_aa_helper_CFLAGS = \ $(NULL) endif WITH_LIBVIRTD endif WITH_SECDRIVER_APPARMOR -EXTRA_DIST += $(SECURITY_DRIVER_APPARMOR_HELPER_SOURCES) +EXTRA_DIST += security/virt-aa-helper.c install-data-local: install-init install-systemd if WITH_LIBVIRTD -- 2.3.5

3 4

[libvirt] [PATCH] tests: Resolve Coverity RESOURCE_LEAK
by John Ferlan 15 Apr '15

15 Apr '15

Commit id 'b77ce18a2' added a new bitmap, but neglected to virBitmapFree it Signed-off-by: John Ferlan <jferlan(a)redhat.com> --- tests/virbitmaptest.c | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/virbitmaptest.c b/tests/virbitmaptest.c index 9a84e4c..a6e9a38 100644 --- a/tests/virbitmaptest.c +++ b/tests/virbitmaptest.c @@ -548,6 +548,7 @@ test10(const void *opaque ATTRIBUTE_UNUSED) virBitmapFree(b1); virBitmapFree(b2); virBitmapFree(b3); + virBitmapFree(b4); return ret; } -- 2.1.0

3 2

[libvirt] [PATCH] virsh: fix regression in 'virsh event' by domain
by Eric Blake 15 Apr '15

15 Apr '15

Commit a0670ae caused a regression in 'virsh event' and 'virsh qemu-monitor-event' - if a user tries to filter the command to a specific domain, an error message is printed: $ virsh event dom --loop error: internal error: virsh qemu-monitor-event: no domain VSH_OT_DATA option and then the command continues as though no domain had been supplied (giving events for ALL domains, instead of the requested one). This is because the code was incorrectly assuming that all "domain" options would be supplied via a mandatory VSH_OT_DATA, even though "domain" is optional for these two commands, so we had changed them to VSH_OT_STRING to quit failing for other reasons (ever since it was decided that VSH_OT_DATA and VSH_OT_STRING should no longer be synonyms). In looking at the situation, though, the code for looking up a domain was making a pointless check for whether the option exists prior to finding the option's string value, as vshCommandOptStringReq does just fine at reporting any errors when looking up a string whether or not the option was present. So this is a case of regression fixing by pure code deletion :) * tools/virsh-domain.c (vshCommandOptDomainBy): Drop useless filter. * tools/virsh-interface.c (vshCommandOptInterfaceBy): Likewise. * tools/virsh-network.c (vshCommandOptNetworkBy): Likewise. * tools/virsh-nwfilter.c (vshCommandOptNWFilterBy): Likewise. * tools/virsh-secret.c (vshCommandOptSecret): Likewise. * tools/virsh.h (vshCmdHasOption): Drop unused function. * tools/virsh.c (vshCmdHasOption): Likewise. Signed-off-by: Eric Blake <eblake(a)redhat.com> --- tools/virsh-domain.c | 3 --- tools/virsh-interface.c | 2 -- tools/virsh-network.c | 3 --- tools/virsh-nwfilter.c | 5 +---- tools/virsh-secret.c | 5 +---- tools/virsh.c | 23 ----------------------- tools/virsh.h | 2 -- 7 files changed, 2 insertions(+), 41 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index bc6054a..2fe3924 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -119,9 +119,6 @@ vshCommandOptDomainBy(vshControl *ctl, const vshCmd *cmd, const char *n = NULL; const char *optname = "domain"; - if (!vshCmdHasOption(ctl, cmd, optname)) - return NULL; - if (vshCommandOptStringReq(ctl, cmd, optname, &n) < 0) return NULL; diff --git a/tools/virsh-interface.c b/tools/virsh-interface.c index 3251e01..b9c60c5 100644 --- a/tools/virsh-interface.c +++ b/tools/virsh-interface.c @@ -53,8 +53,6 @@ vshCommandOptInterfaceBy(vshControl *ctl, const vshCmd *cmd, if (!optname) optname = "interface"; - if (!vshCmdHasOption(ctl, cmd, optname)) - return NULL; if (vshCommandOptStringReq(ctl, cmd, optname, &n) < 0) return NULL; diff --git a/tools/virsh-network.c b/tools/virsh-network.c index 09ee11a..b859b49 100644 --- a/tools/virsh-network.c +++ b/tools/virsh-network.c @@ -42,9 +42,6 @@ vshCommandOptNetworkBy(vshControl *ctl, const vshCmd *cmd, const char *optname = "network"; virCheckFlags(VSH_BYUUID | VSH_BYNAME, NULL); - if (!vshCmdHasOption(ctl, cmd, optname)) - return NULL; - if (vshCommandOptStringReq(ctl, cmd, optname, &n) < 0) return NULL; diff --git a/tools/virsh-nwfilter.c b/tools/virsh-nwfilter.c index e6bef08..63c1c7e 100644 --- a/tools/virsh-nwfilter.c +++ b/tools/virsh-nwfilter.c @@ -1,7 +1,7 @@ /* * virsh-nwfilter.c: Commands to manage network filters * - * Copyright (C) 2005, 2007-2014 Red Hat, Inc. + * Copyright (C) 2005, 2007-2015 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -41,9 +41,6 @@ vshCommandOptNWFilterBy(vshControl *ctl, const vshCmd *cmd, const char *optname = "nwfilter"; virCheckFlags(VSH_BYUUID | VSH_BYNAME, NULL); - if (!vshCmdHasOption(ctl, cmd, optname)) - return NULL; - if (vshCommandOptStringReq(ctl, cmd, optname, &n) < 0) return NULL; diff --git a/tools/virsh-secret.c b/tools/virsh-secret.c index 5065c6f..c6ceabd 100644 --- a/tools/virsh-secret.c +++ b/tools/virsh-secret.c @@ -1,7 +1,7 @@ /* * virsh-secret.c: Commands to manage secret * - * Copyright (C) 2005, 2007-2014 Red Hat, Inc. + * Copyright (C) 2005, 2007-2015 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -41,9 +41,6 @@ vshCommandOptSecret(vshControl *ctl, const vshCmd *cmd, const char **name) const char *n = NULL; const char *optname = "secret"; - if (!vshCmdHasOption(ctl, cmd, optname)) - return NULL; - if (vshCommandOptStringReq(ctl, cmd, optname, &n) < 0) return NULL; diff --git a/tools/virsh.c b/tools/virsh.c index 889a561..4425774 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -1860,29 +1860,6 @@ vshCommandOptArgv(const vshCmd *cmd, const vshCmdOpt *opt) return NULL; } -/* Determine whether CMD->opts includes an option with name OPTNAME. - If not, give a diagnostic and return false. - If so, return true. */ -bool -vshCmdHasOption(vshControl *ctl, const vshCmd *cmd, const char *optname) -{ - /* Iterate through cmd->opts, to ensure that there is an entry - with name OPTNAME and type VSH_OT_DATA. */ - bool found = false; - const vshCmdOpt *opt; - for (opt = cmd->opts; opt; opt = opt->next) { - if (STREQ(opt->def->name, optname) && opt->def->type == VSH_OT_DATA) { - found = true; - break; - } - } - - if (!found) - vshError(ctl, _("internal error: virsh %s: no %s VSH_OT_DATA option"), - cmd->def->name, optname); - return found; -} - /* Parse an optional --timeout parameter in seconds, but store the * value of the timeout in milliseconds. Return -1 on error, 0 if * no timeout was requested, and 1 if timeout was set. */ diff --git a/tools/virsh.h b/tools/virsh.h index e89d315..44a5cd8 100644 --- a/tools/virsh.h +++ b/tools/virsh.h @@ -316,8 +316,6 @@ int vshCommandOptScaledInt(const vshCmd *cmd, const char *name, bool vshCommandOptBool(const vshCmd *cmd, const char *name); const vshCmdOpt *vshCommandOptArgv(const vshCmd *cmd, const vshCmdOpt *opt); -bool vshCmdHasOption(vshControl *ctl, const vshCmd *cmd, const char *optname); - int vshCommandOptTimeoutToMs(vshControl *ctl, const vshCmd *cmd, int *timeout); /* Filter flags for various vshCommandOpt*By() functions */ -- 2.1.0

2 2

[libvirt] [PATCH] node: udev: Remove some redundant error reports
by Peter Krempa 15 Apr '15

15 Apr '15

All the called functions already report an error. --- src/node_device/node_device_udev.c | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 8c39e5f..1025e80 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -128,9 +128,6 @@ static int udevGetDeviceProperty(struct udev_device *udev_device, /* If this allocation is changed, the comment at the beginning * of the function must also be changed. */ if (VIR_STRDUP(*property_value, udev_value) < 0) { - VIR_ERROR(_("Failed to allocate memory for property value for " - "property key '%s' on device with sysname '%s'"), - property_key, udev_device_get_sysname(udev_device)); ret = PROPERTY_ERROR; goto out; } @@ -213,9 +210,6 @@ static int udevGetDeviceSysfsAttr(struct udev_device *udev_device, /* If this allocation is changed, the comment at the beginning * of the function must also be changed. */ if (VIR_STRDUP(*attr_value, udev_value) < 0) { - VIR_ERROR(_("Failed to allocate memory for sysfs attribute value for " - "sysfs attribute '%s' on device with sysname '%s'"), - attr_name, udev_device_get_sysname(udev_device)); ret = PROPERTY_ERROR; goto out; } @@ -1428,11 +1422,8 @@ static int udevAddOneDevice(struct udev_device *device) /* If this is a device change, the old definition will be freed * and the current definition will take its place. */ dev = virNodeDeviceAssignDef(&driver->devs, def); - - if (dev == NULL) { - VIR_ERROR(_("Failed to create device for '%s'"), def->name); + if (dev == NULL) goto out; - } virNodeDeviceObjUnlock(dev); @@ -1686,10 +1677,8 @@ static int udevSetupSystemDev(void) #endif dev = virNodeDeviceAssignDef(&driver->devs, def); - if (dev == NULL) { - VIR_ERROR(_("Failed to create device for '%s'"), def->name); + if (dev == NULL) goto out; - } virNodeDeviceObjUnlock(dev); -- 2.3.5

2 1

[libvirt] [PATCH] virPidFileConstructPath: Drop useless VIR_FREE()
by Michal Privoznik 15 Apr '15

15 Apr '15

If a virAsprintf() within the function fails, we call VIR_FREE() over @rundir variable and jump onto cleanup label, where it is freed again. It doesn't hurt, but not make much sense too. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com> --- Pushed as trivial. src/util/virpidfile.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/util/virpidfile.c b/src/util/virpidfile.c index a77a326..8144ff9 100644 --- a/src/util/virpidfile.c +++ b/src/util/virpidfile.c @@ -556,11 +556,8 @@ virPidFileConstructPath(bool privileged, goto cleanup; } - if (virAsprintf(pidfile, "%s/%s.pid", rundir, progname) < 0) { - VIR_FREE(rundir); + if (virAsprintf(pidfile, "%s/%s.pid", rundir, progname) < 0) goto cleanup; - } - } ret = 0; -- 2.0.5

1 0