[libvirt] [PATCH 0/3] Clean up usage of access(PATH, F_OK)
Peter Krempa (3): util: Declare that virFileExists shall honor errno cgroup: Move [qemu|lxc]GetCpuBWStatus to vicgroup.c and refactor it cleanup: Kill usage of access(PATH, F_OK) in favor of virFileExists() src/libvirt_private.syms | 1 + src/lxc/lxc_driver.c | 52 ++++------------------------------- src/openvz/openvz_conf.c | 2 +- src/parallels/parallels_storage.c | 2 +- src/qemu/qemu_capabilities.c | 4 +-- src/qemu/qemu_cgroup.c | 6 ++-- src/qemu/qemu_driver.c | 51 ++++------------------------------ src/qemu/qemu_process.c | 2 +- src/storage/storage_backend_fs.c | 2 +- src/storage/storage_backend_logical.c | 2 +- src/storage/storage_backend_mpath.c | 8 +----- src/storage/storage_backend_scsi.c | 3 +- src/util/vircgroup.c | 31 ++++++++++++++++++++- src/util/vircgroup.h | 2 ++ src/util/virfile.c | 8 +++++- src/util/virpci.c | 2 +- src/util/virscsi.c | 2 +- src/util/virutil.c | 6 ++-- 18 files changed, 67 insertions(+), 119 deletions(-) -- 1.8.3.2
Explicitly state that some parts of the code may require virFileExists to set or preserve a correct errno so that future modifications don't break. --- src/util/virfile.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/util/virfile.c b/src/util/virfile.c index feac3c9..67adc55 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -1477,6 +1477,12 @@ virFileIsDir(const char *path) return (stat(path, &s) == 0) && S_ISDIR(s.st_mode); } +/** + * virFileExists: Check for presence of file + * @path: Path of file to check + * + * Returns wether file exists. Preserves errno in case it does not exist. + */ bool virFileExists(const char *path) { -- 1.8.3.2
On 09/13/2013 08:19 AM, Peter Krempa wrote:
Explicitly state that some parts of the code may require virFileExists to set or preserve a correct errno so that future modifications don't break. --- src/util/virfile.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/src/util/virfile.c b/src/util/virfile.c index feac3c9..67adc55 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -1477,6 +1477,12 @@ virFileIsDir(const char *path) return (stat(path, &s) == 0) && S_ISDIR(s.st_mode); }
+/** + * virFileExists: Check for presence of file + * @path: Path of file to check + * + * Returns wether file exists. Preserves errno in case it does not exist.
s/wether/whether/ or even: s/wether/true if the/
+ */ bool virFileExists(const char *path) {
ACK once you avoid the typo. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 09/13/13 17:36, Eric Blake wrote:
On 09/13/2013 08:19 AM, Peter Krempa wrote:
Explicitly state that some parts of the code may require virFileExists to set or preserve a correct errno so that future modifications don't break. --- src/util/virfile.c | 6 ++++++ 1 file changed, 6 insertions(+)
...
+/** + * virFileExists: Check for presence of file + * @path: Path of file to check + * + * Returns wether file exists. Preserves errno in case it does not exist.
s/wether/whether/
or even:
s/wether/true if the/
I chose this wording and ...
+ */ bool virFileExists(const char *path) {
ACK once you avoid the typo.
... pushed this patch. Thanks. Peter
The function existed in two identical instances in lxc and qemu. Move it to vircgroup.c and simplify it. Refactor the callers too. --- src/libvirt_private.syms | 1 + src/lxc/lxc_driver.c | 52 +++++------------------------------------------- src/qemu/qemu_driver.c | 51 +++++------------------------------------------ src/util/vircgroup.c | 29 +++++++++++++++++++++++++++ src/util/vircgroup.h | 2 ++ 5 files changed, 42 insertions(+), 93 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 35f0f1b..82e3d6f 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1217,6 +1217,7 @@ virCgroupSetMemory; virCgroupSetMemoryHardLimit; virCgroupSetMemorySoftLimit; virCgroupSetMemSwapHardLimit; +virCgroupSupportsCpuBW; # util/virclosecallbacks.h diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index b587c22..87ced95 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -1574,45 +1574,10 @@ static char *lxcConnectGetHostname(virConnectPtr conn) } - -/* - * check whether the host supports CFS bandwidth - * - * Return 1 when CFS bandwidth is supported, 0 when CFS bandwidth is not - * supported, -1 on error. - */ -static int lxcGetCpuBWStatus(virCgroupPtr cgroup) -{ - char *cfs_period_path = NULL; - int ret = -1; - - if (!cgroup) - return 0; - - if (virCgroupPathOfController(cgroup, VIR_CGROUP_CONTROLLER_CPU, - "cpu.cfs_period_us", &cfs_period_path) < 0) { - VIR_INFO("cannot get the path of cgroup CPU controller"); - ret = 0; - goto cleanup; - } - - if (access(cfs_period_path, F_OK) < 0) { - ret = 0; - } else { - ret = 1; - } - -cleanup: - VIR_FREE(cfs_period_path); - return ret; -} - - static char *lxcDomainGetSchedulerType(virDomainPtr dom, int *nparams) { char *ret = NULL; - int rc; virDomainObjPtr vm; virLXCDomainObjPrivatePtr priv; @@ -1639,13 +1604,10 @@ static char *lxcDomainGetSchedulerType(virDomainPtr dom, } if (nparams) { - rc = lxcGetCpuBWStatus(priv->cgroup); - if (rc < 0) - goto cleanup; - else if (rc == 0) - *nparams = 1; - else + if (virCgroupSupportsCpuBW(priv->cgroup)) *nparams = 3; + else + *nparams = 1; } ignore_value(VIR_STRDUP(ret, "posix")); @@ -1872,12 +1834,8 @@ lxcDomainGetSchedulerParametersFlags(virDomainPtr dom, if (virDomainGetSchedulerParametersFlagsEnsureACL(dom->conn, vm->def) < 0) goto cleanup; - if (*nparams > 1) { - rc = lxcGetCpuBWStatus(priv->cgroup); - if (rc < 0) - goto cleanup; - cpu_bw_status = !!rc; - } + if (*nparams > 1) + cpu_bw_status = virCgroupSupportsCpuBW(priv->cgroup); if (!(caps = virLXCDriverGetCapabilities(driver, false))) goto cleanup; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ae1948f..0caeb08 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7325,44 +7325,10 @@ cleanup: } -/* - * check whether the host supports CFS bandwidth - * - * Return 1 when CFS bandwidth is supported, 0 when CFS bandwidth is not - * supported, -1 on error. - */ -static int qemuGetCpuBWStatus(virCgroupPtr cgroup) -{ - char *cfs_period_path = NULL; - int ret = -1; - - if (!cgroup) - return 0; - - if (virCgroupPathOfController(cgroup, VIR_CGROUP_CONTROLLER_CPU, - "cpu.cfs_period_us", &cfs_period_path) < 0) { - VIR_INFO("cannot get the path of cgroup CPU controller"); - ret = 0; - goto cleanup; - } - - if (access(cfs_period_path, F_OK) < 0) { - ret = 0; - } else { - ret = 1; - } - -cleanup: - VIR_FREE(cfs_period_path); - return ret; -} - - static char *qemuDomainGetSchedulerType(virDomainPtr dom, int *nparams) { char *ret = NULL; - int rc; virDomainObjPtr vm = NULL; qemuDomainObjPrivatePtr priv; @@ -7389,13 +7355,10 @@ static char *qemuDomainGetSchedulerType(virDomainPtr dom, } if (nparams) { - rc = qemuGetCpuBWStatus(priv->cgroup); - if (rc < 0) - goto cleanup; - else if (rc == 0) - *nparams = 1; - else + if (virCgroupSupportsCpuBW(priv->cgroup)) *nparams = 5; + else + *nparams = 1; } ignore_value(VIR_STRDUP(ret, "posix")); @@ -8728,12 +8691,8 @@ qemuDomainGetSchedulerParametersFlags(virDomainPtr dom, if (virDomainGetSchedulerParametersFlagsEnsureACL(dom->conn, vm->def) < 0) goto cleanup; - if (*nparams > 1) { - rc = qemuGetCpuBWStatus(priv->cgroup); - if (rc < 0) - goto cleanup; - cpu_bw_status = !!rc; - } + if (*nparams > 1) + cpu_bw_status = virCgroupSupportsCpuBW(priv->cgroup); if (!(caps = virQEMUDriverGetCapabilities(driver, false))) goto cleanup; diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 626bbc6..0b4e901 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -3647,3 +3647,32 @@ virCgroupIsolateMount(virCgroupPtr group ATTRIBUTE_UNUSED, } #endif /* !VIR_CGROUP_SUPPORTED */ + +/** + * virCgroupSupportsCpuBW(): + * Check whether the host supports CFS bandwidth. + * + * Return true when CFS bandwidth is supported, + * false when CFS bandwidth is not supported. + */ +bool +virCgroupSupportsCpuBW(virCgroupPtr cgroup) +{ + char *path = NULL; + int ret = false; + + if (!cgroup) + return false; + + if (virCgroupPathOfController(cgroup, VIR_CGROUP_CONTROLLER_CPU, + "cpu.cfs_period_us", &path) < 0) { + virResetLastError(); + goto cleanup; + } + + ret = virFileExists(path); + +cleanup: + VIR_FREE(path); + return ret; +} diff --git a/src/util/vircgroup.h b/src/util/vircgroup.h index 7bb4b2a..835eb30 100644 --- a/src/util/vircgroup.h +++ b/src/util/vircgroup.h @@ -207,4 +207,6 @@ int virCgroupIsolateMount(virCgroupPtr group, const char *oldroot, const char *mountopts); +bool virCgroupSupportsCpuBW(virCgroupPtr cgroup); + #endif /* __VIR_CGROUP_H__ */ -- 1.8.3.2
On 09/13/2013 08:19 AM, Peter Krempa wrote: s/vicgroup/vircgroup/ in the subject
The function existed in two identical instances in lxc and qemu. Move it to vircgroup.c and simplify it. Refactor the callers too. ---
+++ b/src/util/vircgroup.c @@ -3647,3 +3647,32 @@ virCgroupIsolateMount(virCgroupPtr group ATTRIBUTE_UNUSED, }
#endif /* !VIR_CGROUP_SUPPORTED */ + +/** + * virCgroupSupportsCpuBW(): + * Check whether the host supports CFS bandwidth. + * + * Return true when CFS bandwidth is supported, + * false when CFS bandwidth is not supported. + */ +bool +virCgroupSupportsCpuBW(virCgroupPtr cgroup) +{ + char *path = NULL; + int ret = false; + + if (!cgroup) + return false; + + if (virCgroupPathOfController(cgroup, VIR_CGROUP_CONTROLLER_CPU, + "cpu.cfs_period_us", &path) < 0) {
When cgroups are not compiled in, this function call always results in an error;
+ virResetLastError();
that you will just ignore. Rather than pollute the logs, it may be nicer to put the REAL implementation inside the #if, then provide the #else a version that just silently returns false without spamming the logs. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Semantics of the libvirt helper are more clear. This change also allows to clean up some pieces of code. --- src/openvz/openvz_conf.c | 2 +- src/parallels/parallels_storage.c | 2 +- src/qemu/qemu_capabilities.c | 4 ++-- src/qemu/qemu_cgroup.c | 6 +++--- src/qemu/qemu_process.c | 2 +- src/storage/storage_backend_fs.c | 2 +- src/storage/storage_backend_logical.c | 2 +- src/storage/storage_backend_mpath.c | 8 +------- src/storage/storage_backend_scsi.c | 3 +-- src/util/vircgroup.c | 2 +- src/util/virfile.c | 2 +- src/util/virpci.c | 2 +- src/util/virscsi.c | 2 +- src/util/virutil.c | 6 +++--- 14 files changed, 19 insertions(+), 26 deletions(-) diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c index 93f2377..0dbaa4a 100644 --- a/src/openvz/openvz_conf.c +++ b/src/openvz/openvz_conf.c @@ -940,7 +940,7 @@ openvzLocateConfDir(void) char *ret = NULL; while (conf_dir_list[i]) { - if (!access(conf_dir_list[i], F_OK)) { + if (virFileExists(conf_dir_list[i])) { ignore_value(VIR_STRDUP(ret, conf_dir_list[i])); goto cleanup; } diff --git a/src/parallels/parallels_storage.c b/src/parallels/parallels_storage.c index 44246a7..bb5066f 100644 --- a/src/parallels/parallels_storage.c +++ b/src/parallels/parallels_storage.c @@ -362,7 +362,7 @@ static int parallelsFindVmVolumes(virStoragePoolObjPtr pool, "DiskDescriptor", ".xml"))) goto cleanup; - if (access(diskDescPath, F_OK)) + if (!virFileExists(diskDescPath)) continue; /* here we know, that ent->d_name is a disk image directory */ diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index d94188a..d830e2a 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -731,13 +731,13 @@ virQEMUCapsInitGuest(virCapsPtr caps, if (!binary) return 0; - if (access("/dev/kvm", F_OK) == 0 && + if (virFileExists("/dev/kvm") && (virQEMUCapsGet(qemubinCaps, QEMU_CAPS_KVM) || virQEMUCapsGet(qemubinCaps, QEMU_CAPS_ENABLE_KVM) || kvmbin)) haskvm = true; - if (access("/dev/kqemu", F_OK) == 0 && + if (virFileExists("/dev/kqemu") && virQEMUCapsGet(qemubinCaps, QEMU_CAPS_KQEMU)) haskqemu = true; diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index cf41c33..f95c7f2 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -33,6 +33,7 @@ #include "domain_audit.h" #include "virscsi.h" #include "virstring.h" +#include "virfile.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -501,9 +502,8 @@ qemuSetupDevicesCgroup(virQEMUDriverPtr driver, } for (i = 0; deviceACL[i] != NULL; i++) { - if (access(deviceACL[i], F_OK) < 0) { - VIR_DEBUG("Ignoring non-existant device %s", - deviceACL[i]); + if (!virFileExists(deviceACL[i])) { + VIR_DEBUG("Ignoring non-existant device %s", deviceACL[i]); continue; } diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index c7cec5a..dd16f6c 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3632,7 +3632,7 @@ int qemuProcessStart(virConnectPtr conn, if (vm->def->virtType == VIR_DOMAIN_VIRT_KVM) { VIR_DEBUG("Checking for KVM availability"); - if (access("/dev/kvm", F_OK) != 0) { + if (!virFileExists("/dev/kvm")) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Domain requires KVM, but it is not available. " "Check that virtualization is enabled in the host BIOS, " diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index d305b06..956cfce 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -503,7 +503,7 @@ virStorageBackendFileSystemCheck(virConnectPtr conn ATTRIBUTE_UNUSED, { *isActive = false; if (pool->def->type == VIR_STORAGE_POOL_DIR) { - if (access(pool->def->target.path, F_OK) == 0) + if (virFileExists(pool->def->target.path)) *isActive = true; #if WITH_STORAGE_FS } else { diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index 0b14679..a1a37a1 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -453,7 +453,7 @@ virStorageBackendLogicalCheckPool(virConnectPtr conn ATTRIBUTE_UNUSED, virStoragePoolObjPtr pool, bool *isActive) { - *isActive = (access(pool->def->target.path, F_OK) == 0); + *isActive = virFileExists(pool->def->target.path); return 0; } diff --git a/src/storage/storage_backend_mpath.c b/src/storage/storage_backend_mpath.c index 8333f18..9a19484 100644 --- a/src/storage/storage_backend_mpath.c +++ b/src/storage/storage_backend_mpath.c @@ -287,13 +287,7 @@ virStorageBackendMpathCheckPool(virConnectPtr conn ATTRIBUTE_UNUSED, virStoragePoolObjPtr pool ATTRIBUTE_UNUSED, bool *isActive) { - const char *path = "/dev/mpath"; - - *isActive = false; - - if (access(path, F_OK) == 0) - *isActive = true; - + *isActive = virFileExists("/dev/mpath"); return 0; } diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index 8cb762a..5769799 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -700,8 +700,7 @@ virStorageBackendSCSICheckPool(virConnectPtr conn ATTRIBUTE_UNUSED, if (virAsprintf(&path, "/sys/class/scsi_host/host%d", host) < 0) goto cleanup; - if (access(path, F_OK) == 0) - *isActive = true; + *isActive = virFileExists(path); ret = 0; cleanup: diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 0b4e901..2f73e47 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -905,7 +905,7 @@ virCgroupMakeGroup(virCgroupPtr parent, sa_assert(group->controllers[i].mountPoint); VIR_DEBUG("Make controller %s", path); - if (access(path, F_OK) != 0) { + if (!virFileExists(path)) { if (!create || mkdir(path, 0755) < 0) { /* With a kernel that doesn't support multi-level directory diff --git a/src/util/virfile.c b/src/util/virfile.c index 67adc55..fe769dd 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -735,7 +735,7 @@ virFileNBDDeviceIsBusy(const char *devname) devname) < 0) return -1; - if (access(path, F_OK) < 0) { + if (!virFileExists(path)) { if (errno == ENOENT) ret = 0; else diff --git a/src/util/virpci.c b/src/util/virpci.c index 92927aa..65d7168 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -1501,7 +1501,7 @@ virPCIDeviceNew(unsigned int domain, dev->name) < 0) goto error; - if (access(dev->path, F_OK) != 0) { + if (!virFileExists(dev->path)) { virReportSystemError(errno, _("Device %s not found: could not access %s"), dev->name, dev->path); diff --git a/src/util/virscsi.c b/src/util/virscsi.c index 32e438b..43658c0 100644 --- a/src/util/virscsi.c +++ b/src/util/virscsi.c @@ -216,7 +216,7 @@ virSCSIDeviceNew(const char *adapter, virAsprintf(&dev->sg_path, "/dev/%s", sg) < 0) goto cleanup; - if (access(dev->sg_path, F_OK) != 0) { + if (!virFileExists(dev->sg_path)) { virReportSystemError(errno, _("SCSI device '%s': could not access %s"), dev->name, dev->sg_path); diff --git a/src/util/virutil.c b/src/util/virutil.c index 39d4717..d9e0bc4 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -1711,7 +1711,7 @@ virIsCapableFCHost(const char *sysfs_prefix, host) < 0) return false; - if (access(sysfs_path, F_OK) == 0) + if (virFileExists(sysfs_path)) ret = true; VIR_FREE(sysfs_path); @@ -1740,8 +1740,8 @@ virIsCapableVport(const char *sysfs_prefix, "vport_create") < 0) goto cleanup; - if ((access(fc_host_path, F_OK) == 0) || - (access(scsi_host_path, F_OK) == 0)) + if (virFileExists(fc_host_path) || + virFileExists(scsi_host_path)) ret = true; cleanup: -- 1.8.3.2
On 09/13/2013 04:19 PM, Peter Krempa wrote:
Semantics of the libvirt helper are more clear. This change also allows to clean up some pieces of code. --- src/openvz/openvz_conf.c | 2 +- src/parallels/parallels_storage.c | 2 +- src/qemu/qemu_capabilities.c | 4 ++-- src/qemu/qemu_cgroup.c | 6 +++--- src/qemu/qemu_process.c | 2 +- src/storage/storage_backend_fs.c | 2 +- src/storage/storage_backend_logical.c | 2 +- src/storage/storage_backend_mpath.c | 8 +------- src/storage/storage_backend_scsi.c | 3 +-- src/util/vircgroup.c | 2 +- src/util/virfile.c | 2 +- src/util/virpci.c | 2 +- src/util/virscsi.c | 2 +- src/util/virutil.c | 6 +++--- 14 files changed, 19 insertions(+), 26 deletions(-)
[...]
diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index d305b06..956cfce 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -503,7 +503,7 @@ virStorageBackendFileSystemCheck(virConnectPtr conn ATTRIBUTE_UNUSED, { *isActive = false; if (pool->def->type == VIR_STORAGE_POOL_DIR) { - if (access(pool->def->target.path, F_OK) == 0) + if (virFileExists(pool->def->target.path)) *isActive = true;
Why didn't you use the same (nicer) way here, when you used it everywhere else. I'm talking about '*isActive = virFileExists(...)'. ACK either way, looking forward to [12]/3 ;-) Martin P.S.: If you'll need the answer urgently, ping me and I'll see them in the archives.
On 09/13/13 17:31, Martin Kletzander wrote:
On 09/13/2013 04:19 PM, Peter Krempa wrote:
Semantics of the libvirt helper are more clear. This change also allows to clean up some pieces of code. ---
[...]
diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index d305b06..956cfce 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -503,7 +503,7 @@ virStorageBackendFileSystemCheck(virConnectPtr conn ATTRIBUTE_UNUSED, { *isActive = false; if (pool->def->type == VIR_STORAGE_POOL_DIR) { - if (access(pool->def->target.path, F_OK) == 0) + if (virFileExists(pool->def->target.path)) *isActive = true;
Why didn't you use the same (nicer) way here, when you used it everywhere else. I'm talking about '*isActive = virFileExists(...)'.
ACK either way, looking forward to [12]/3 ;-)
Martin
P.S.: If you'll need the answer urgently, ping me and I'll see them in the archives.
I cleaned up the part you've pointed out and pushed this patch. Thanks Peter
participants (3)
-
Eric Blake -
Martin Kletzander -
Peter Krempa