[libvirt] [PATCH] Fix return value semantic of virFileMakePath
Some callers expected virFileMakePath to set errno, some expected it to return an errno value. Unify this to return 0 on success and -1 on error. Set errno to report detailed error information. Also Make virFileMakePath report an error when stat fails with an errno different from ENOENT. --- src/conf/domain_conf.c | 2 +- src/conf/network_conf.c | 5 +-- src/conf/nwfilter_conf.c | 11 +++----- src/conf/storage_conf.c | 6 +--- src/libxl/libxl_driver.c | 14 ++++----- src/lxc/lxc_container.c | 18 ++++++------ src/lxc/lxc_controller.c | 2 +- src/lxc/lxc_driver.c | 10 +++---- src/network/bridge_driver.c | 25 ++++++++--------- src/qemu/qemu_driver.c | 28 ++++++++----------- src/qemu/qemu_process.c | 2 +- src/storage/storage_backend_fs.c | 4 +- src/storage/storage_driver.c | 6 +--- src/uml/uml_driver.c | 10 +++---- src/util/dnsmasq.c | 5 +-- src/util/util.c | 56 ++++++++++++++++++++++++------------- 16 files changed, 100 insertions(+), 104 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 109a947..64c5636 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9986,7 +9986,7 @@ int virDomainSaveXML(const char *configDir, if ((configFile = virDomainConfigFile(configDir, def->name)) == NULL) goto cleanup; - if (virFileMakePath(configDir)) { + if (virFileMakePath(configDir) < 0) { virReportSystemError(errno, _("cannot create config directory '%s'"), configDir); diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 45ddee2..ae479bf 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -1114,13 +1114,12 @@ int virNetworkSaveXML(const char *configDir, char *configFile = NULL; int fd = -1, ret = -1; size_t towrite; - int err; if ((configFile = virNetworkConfigFile(configDir, def->name)) == NULL) goto cleanup; - if ((err = virFileMakePath(configDir))) { - virReportSystemError(err, + if (virFileMakePath(configDir) < 0) { + virReportSystemError(errno, _("cannot create config directory '%s'"), configDir); goto cleanup; diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index 127d4be..036c61a 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -2184,13 +2184,12 @@ int virNWFilterSaveXML(const char *configDir, char *configFile = NULL; int fd = -1, ret = -1; size_t towrite; - int err; if ((configFile = virNWFilterConfigFile(configDir, def->name)) == NULL) goto cleanup; - if ((err = virFileMakePath(configDir))) { - virReportSystemError(err, + if (virFileMakePath(configDir) < 0) { + virReportSystemError(errno, _("cannot create config directory '%s'"), configDir); goto cleanup; @@ -2574,10 +2573,8 @@ virNWFilterObjSaveDef(virNWFilterDriverStatePtr driver, ssize_t towrite; if (!nwfilter->configFile) { - int err; - - if ((err = virFileMakePath(driver->configDir))) { - virReportSystemError(err, + if (virFileMakePath(driver->configDir) < 0) { + virReportSystemError(errno, _("cannot create config directory %s"), driver->configDir); return -1; diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index ca86f19..f6f8be1 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1512,10 +1512,8 @@ virStoragePoolObjSaveDef(virStorageDriverStatePtr driver, ssize_t towrite; if (!pool->configFile) { - int err; - - if ((err = virFileMakePath(driver->configDir))) { - virReportSystemError(err, + if (virFileMakePath(driver->configDir) < 0) { + virReportSystemError(errno, _("cannot create config directory %s"), driver->configDir); return -1; diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 7fd257d..ade69d8 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -914,25 +914,25 @@ libxlStartup(int privileged) { "%s", LIBXL_SAVE_DIR) == -1) goto out_of_memory; - if (virFileMakePath(libxl_driver->logDir) != 0) { + if (virFileMakePath(libxl_driver->logDir) < 0) { char ebuf[1024]; VIR_ERROR(_("Failed to create log dir '%s': %s"), libxl_driver->logDir, virStrerror(errno, ebuf, sizeof ebuf)); goto error; } - if (virFileMakePath(libxl_driver->stateDir) != 0) { + if (virFileMakePath(libxl_driver->stateDir) < 0) { char ebuf[1024]; VIR_ERROR(_("Failed to create state dir '%s': %s"), libxl_driver->stateDir, virStrerror(errno, ebuf, sizeof ebuf)); goto error; } - if (virFileMakePath(libxl_driver->libDir) != 0) { + if (virFileMakePath(libxl_driver->libDir) < 0) { char ebuf[1024]; VIR_ERROR(_("Failed to create lib dir '%s': %s"), libxl_driver->libDir, virStrerror(errno, ebuf, sizeof ebuf)); goto error; } - if (virFileMakePath(libxl_driver->saveDir) != 0) { + if (virFileMakePath(libxl_driver->saveDir) < 0) { char ebuf[1024]; VIR_ERROR(_("Failed to create save dir '%s': %s"), libxl_driver->saveDir, virStrerror(errno, ebuf, sizeof ebuf)); @@ -3389,10 +3389,8 @@ libxlDomainSetAutostart(virDomainPtr dom, int autostart) goto cleanup; if (autostart) { - int err; - - if ((err = virFileMakePath(driver->autostartDir))) { - virReportSystemError(err, + if (virFileMakePath(driver->autostartDir) < 0) { + virReportSystemError(errno, _("cannot create autostart directory %s"), driver->autostartDir); goto cleanup; diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index 7924858..ef8469c 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -308,7 +308,7 @@ static int lxcContainerChildMountSort(const void *a, const void *b) static int lxcContainerPivotRoot(virDomainFSDefPtr root) { - int rc, ret; + int ret; char *oldroot = NULL, *newroot = NULL; ret = -1; @@ -325,8 +325,8 @@ static int lxcContainerPivotRoot(virDomainFSDefPtr root) goto err; } - if ((rc = virFileMakePath(oldroot)) != 0) { - virReportSystemError(rc, + if (virFileMakePath(oldroot) < 0) { + virReportSystemError(errno, _("Failed to create %s"), oldroot); goto err; @@ -347,8 +347,8 @@ static int lxcContainerPivotRoot(virDomainFSDefPtr root) goto err; } - if ((rc = virFileMakePath(newroot)) != 0) { - virReportSystemError(rc, + if (virFileMakePath(newroot) < 0) { + virReportSystemError(errno, _("Failed to create %s"), newroot); goto err; @@ -415,7 +415,7 @@ static int lxcContainerMountBasicFS(virDomainFSDefPtr root) } for (i = 0 ; i < ARRAY_CARDINALITY(mnts) ; i++) { - if (virFileMakePath(mnts[i].dst) != 0) { + if (virFileMakePath(mnts[i].dst) < 0) { virReportSystemError(errno, _("Failed to mkdir %s"), mnts[i].src); @@ -429,8 +429,8 @@ static int lxcContainerMountBasicFS(virDomainFSDefPtr root) } } - if ((rc = virFileMakePath("/dev/pts") != 0)) { - virReportSystemError(rc, "%s", + if (virFileMakePath("/dev/pts") < 0) { + virReportSystemError(errno, "%s", _("Cannot create /dev/pts")); goto cleanup; } @@ -531,7 +531,7 @@ static int lxcContainerMountNewFS(virDomainDefPtr vmDef) return -1; } - if (virFileMakePath(vmDef->fss[i]->dst) != 0) { + if (virFileMakePath(vmDef->fss[i]->dst) < 0) { virReportSystemError(errno, _("Failed to create %s"), vmDef->fss[i]->dst); diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index 7d60090..31c7d4f 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -690,7 +690,7 @@ lxcControllerRun(virDomainDefPtr def, goto cleanup; } - if (virFileMakePath(devpts) != 0) { + if (virFileMakePath(devpts) < 0) { virReportSystemError(errno, _("Failed to make path %s"), devpts); diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 7220a9b..ffcfe4d 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -1502,8 +1502,8 @@ static int lxcVmStart(virConnectPtr conn, return -1; } - if ((r = virFileMakePath(driver->logDir)) != 0) { - virReportSystemError(r, + if (virFileMakePath(driver->logDir) < 0) { + virReportSystemError(errno, _("Cannot create log directory '%s'"), driver->logDir); return -1; @@ -2539,10 +2539,8 @@ static int lxcDomainSetAutostart(virDomainPtr dom, goto cleanup; if (autostart) { - int err; - - if ((err = virFileMakePath(driver->autostartDir))) { - virReportSystemError(err, + if (virFileMakePath(driver->autostartDir) < 0) { + virReportSystemError(errno, _("Cannot create autostart directory %s"), driver->autostartDir); goto cleanup; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 660dd65..170bf98 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -692,17 +692,16 @@ networkStartDhcpDaemon(virNetworkObjPtr network) virCommandPtr cmd = NULL; char *pidfile = NULL; int ret = -1; - int err; dnsmasqContext *dctx = NULL; - if ((err = virFileMakePath(NETWORK_PID_DIR)) != 0) { - virReportSystemError(err, + if (virFileMakePath(NETWORK_PID_DIR) < 0) { + virReportSystemError(errno, _("cannot create directory %s"), NETWORK_PID_DIR); goto cleanup; } - if ((err = virFileMakePath(NETWORK_STATE_DIR)) != 0) { - virReportSystemError(err, + if (virFileMakePath(NETWORK_STATE_DIR) < 0) { + virReportSystemError(errno, _("cannot create directory %s"), NETWORK_STATE_DIR); goto cleanup; @@ -713,8 +712,8 @@ networkStartDhcpDaemon(virNetworkObjPtr network) goto cleanup; } - if ((err = virFileMakePath(DNSMASQ_STATE_DIR)) != 0) { - virReportSystemError(err, + if (virFileMakePath(DNSMASQ_STATE_DIR) < 0) { + virReportSystemError(errno, _("cannot create directory %s"), DNSMASQ_STATE_DIR); goto cleanup; @@ -777,14 +776,14 @@ networkStartRadvd(virNetworkObjPtr network) goto cleanup; } - if ((err = virFileMakePath(NETWORK_PID_DIR)) != 0) { - virReportSystemError(err, + if (virFileMakePath(NETWORK_PID_DIR) < 0) { + virReportSystemError(errno, _("cannot create directory %s"), NETWORK_PID_DIR); goto cleanup; } - if ((err = virFileMakePath(RADVD_STATE_DIR)) != 0) { - virReportSystemError(err, + if (virFileMakePath(RADVD_STATE_DIR) < 0) { + virReportSystemError(errno, _("cannot create directory %s"), RADVD_STATE_DIR); goto cleanup; @@ -2500,7 +2499,7 @@ static int networkSetAutostart(virNetworkPtr net, struct network_driver *driver = net->conn->networkPrivateData; virNetworkObjPtr network; char *configFile = NULL, *autostartLink = NULL; - int ret = -1; + int ret = -1, err; networkDriverLock(driver); network = virNetworkFindByUUID(&driver->networks, net->uuid); @@ -2526,7 +2525,7 @@ static int networkSetAutostart(virNetworkPtr net, goto cleanup; if (autostart) { - if (virFileMakePath(driver->networkAutostartDir)) { + if (virFileMakePath(driver->networkAutostartDir) < 0) { virReportSystemError(errno, _("cannot create autostart directory '%s'"), driver->networkAutostartDir); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index aab3ab9..0f645fa 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -469,37 +469,37 @@ qemudStartup(int privileged) { goto out_of_memory; } - if (virFileMakePath(qemu_driver->stateDir) != 0) { + if (virFileMakePath(qemu_driver->stateDir) < 0) { char ebuf[1024]; VIR_ERROR(_("Failed to create state dir '%s': %s"), qemu_driver->stateDir, virStrerror(errno, ebuf, sizeof ebuf)); goto error; } - if (virFileMakePath(qemu_driver->libDir) != 0) { + if (virFileMakePath(qemu_driver->libDir) < 0) { char ebuf[1024]; VIR_ERROR(_("Failed to create lib dir '%s': %s"), qemu_driver->libDir, virStrerror(errno, ebuf, sizeof ebuf)); goto error; } - if (virFileMakePath(qemu_driver->cacheDir) != 0) { + if (virFileMakePath(qemu_driver->cacheDir) < 0) { char ebuf[1024]; VIR_ERROR(_("Failed to create cache dir '%s': %s"), qemu_driver->cacheDir, virStrerror(errno, ebuf, sizeof ebuf)); goto error; } - if (virFileMakePath(qemu_driver->saveDir) != 0) { + if (virFileMakePath(qemu_driver->saveDir) < 0) { char ebuf[1024]; VIR_ERROR(_("Failed to create save dir '%s': %s"), qemu_driver->saveDir, virStrerror(errno, ebuf, sizeof ebuf)); goto error; } - if (virFileMakePath(qemu_driver->snapshotDir) != 0) { + if (virFileMakePath(qemu_driver->snapshotDir) < 0) { char ebuf[1024]; VIR_ERROR(_("Failed to create save dir '%s': %s"), qemu_driver->snapshotDir, virStrerror(errno, ebuf, sizeof ebuf)); goto error; } - if (virFileMakePath(qemu_driver->autoDumpPath) != 0) { + if (virFileMakePath(qemu_driver->autoDumpPath) < 0) { char ebuf[1024]; VIR_ERROR(_("Failed to create dump dir '%s': %s"), qemu_driver->autoDumpPath, virStrerror(errno, ebuf, sizeof ebuf)); @@ -586,8 +586,8 @@ qemudStartup(int privileged) { if (virAsprintf(&mempath, "%s/libvirt/qemu", qemu_driver->hugetlbfs_mount) < 0) goto out_of_memory; - if ((rc = virFileMakePath(mempath)) != 0) { - virReportSystemError(rc, + if (virFileMakePath(mempath) < 0) { + virReportSystemError(errno, _("unable to create hugepage path %s"), mempath); VIR_FREE(mempath); goto error; @@ -5017,10 +5017,8 @@ static int qemudDomainSetAutostart(virDomainPtr dom, goto cleanup; if (autostart) { - int err; - - if ((err = virFileMakePath(driver->autostartDir))) { - virReportSystemError(err, + if (virFileMakePath(driver->autostartDir) < 0) { + virReportSystemError(errno, _("cannot create autostart directory %s"), driver->autostartDir); goto cleanup; @@ -7483,7 +7481,6 @@ static int qemuDomainSnapshotWriteMetadata(virDomainObjPtr vm, int ret = -1; char *snapDir = NULL; char *snapFile = NULL; - int err; char uuidstr[VIR_UUID_STRING_BUFLEN]; virUUIDFormat(vm->def->uuid, uuidstr); @@ -7497,9 +7494,8 @@ static int qemuDomainSnapshotWriteMetadata(virDomainObjPtr vm, virReportOOMError(); goto cleanup; } - err = virFileMakePath(snapDir); - if (err != 0) { - virReportSystemError(err, _("cannot create snapshot directory '%s'"), + if (virFileMakePath(snapDir) < 0) { + virReportSystemError(errno, _("cannot create snapshot directory '%s'"), snapDir); goto cleanup; } diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index f2c439b..c9145cb 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2448,7 +2448,7 @@ int qemuProcessStart(virConnectPtr conn, } } - if (virFileMakePath(driver->logDir) != 0) { + if (virFileMakePath(driver->logDir) < 0) { virReportSystemError(errno, _("cannot create log directory %s"), driver->logDir); diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index f2912f0..d87401f 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -559,8 +559,8 @@ virStorageBackendFileSystemBuild(virConnectPtr conn ATTRIBUTE_UNUSED, /* assure all directories in the path prior to the final dir * exist, with default uid/gid/mode. */ *p = '\0'; - if ((err = virFileMakePath(parent)) != 0) { - virReportSystemError(err, _("cannot create path '%s'"), + if (virFileMakePath(parent) < 0) { + virReportSystemError(errno, _("cannot create path '%s'"), parent); goto error; } diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index f9652f8..a990118 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1019,10 +1019,8 @@ storagePoolSetAutostart(virStoragePoolPtr obj, if (pool->autostart != autostart) { if (autostart) { - int err; - - if ((err = virFileMakePath(driver->autostartDir))) { - virReportSystemError(err, + if (virFileMakePath(driver->autostartDir) < 0) { + virReportSystemError(errno, _("cannot create autostart directory %s"), driver->autostartDir); goto cleanup; diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index a71ea21..f0f053b 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -420,7 +420,7 @@ umlStartup(int privileged) goto error; } - if (virFileMakePath(uml_driver->monitorDir) != 0) { + if (virFileMakePath(uml_driver->monitorDir) < 0) { char ebuf[1024]; VIR_ERROR(_("Failed to create monitor directory %s: %s"), uml_driver->monitorDir, virStrerror(errno, ebuf, sizeof ebuf)); @@ -839,7 +839,7 @@ static int umlStartVMDaemon(virConnectPtr conn, return -1; } - if (virFileMakePath(driver->logDir) != 0) { + if (virFileMakePath(driver->logDir) < 0) { virReportSystemError(errno, _("cannot create log directory %s"), driver->logDir); @@ -2004,10 +2004,8 @@ static int umlDomainSetAutostart(virDomainPtr dom, goto cleanup; if (autostart) { - int err; - - if ((err = virFileMakePath(driver->autostartDir))) { - virReportSystemError(err, + if (virFileMakePath(driver->autostartDir) < 0) { + virReportSystemError(errno, _("cannot create autostart directory %s"), driver->autostartDir); goto cleanup; diff --git a/src/util/dnsmasq.c b/src/util/dnsmasq.c index aadca10..55db96b 100644 --- a/src/util/dnsmasq.c +++ b/src/util/dnsmasq.c @@ -523,11 +523,10 @@ dnsmasqAddHost(dnsmasqContext *ctx, int dnsmasqSave(const dnsmasqContext *ctx) { - int err; int ret = 0; - if ((err = virFileMakePath(ctx->config_dir))) { - virReportSystemError(err, _("cannot create config directory '%s'"), + if (virFileMakePath(ctx->config_dir) < 0) { + virReportSystemError(errno, _("cannot create config directory '%s'"), ctx->config_dir); return -1; } diff --git a/src/util/util.c b/src/util/util.c index 13973c3..482ca6b 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -1010,66 +1010,80 @@ int virDirCreate(const char *path ATTRIBUTE_UNUSED, } #endif /* WIN32 */ -static int virFileMakePathHelper(char *path) { +static int virFileMakePathHelper(char *path) +{ struct stat st; char *p = NULL; - int err; if (stat(path, &st) >= 0) return 0; + else if (errno != ENOENT) + return -1; - if ((p = strrchr(path, '/')) == NULL) - return EINVAL; + if ((p = strrchr(path, '/')) == NULL) { + errno = EINVAL; + return -1; + } if (p != path) { *p = '\0'; - err = virFileMakePathHelper(path); + + if (virFileMakePathHelper(path) < 0) + return -1; + *p = '/'; - if (err != 0) - return err; } - if (mkdir(path, 0777) < 0 && errno != EEXIST) { - return errno; - } + if (mkdir(path, 0777) < 0 && errno != EEXIST) + return -1; + return 0; } +/** + * Creates the given path with mode 0777 if it's not already existing + * completely. + * + * Returns 0 on success, or -1 if an error occurred (in which case, errno + * is set appropriately). + */ int virFileMakePath(const char *path) { + int ret = -1; struct stat st; char *parent = NULL; char *p; - int err = 0; if (stat(path, &st) >= 0) + return 0; + else if (errno != ENOENT) goto cleanup; if ((parent = strdup(path)) == NULL) { - err = ENOMEM; + errno = ENOMEM; goto cleanup; } if ((p = strrchr(parent, '/')) == NULL) { - err = EINVAL; + errno = EINVAL; goto cleanup; } if (p != parent) { *p = '\0'; - if ((err = virFileMakePathHelper(parent)) != 0) { + + if (virFileMakePathHelper(parent) < 0) goto cleanup; - } } - if (mkdir(path, 0777) < 0 && errno != EEXIST) { - err = errno; + if (mkdir(path, 0777) < 0 && errno != EEXIST) goto cleanup; - } + + ret = 0; cleanup: VIR_FREE(parent); - return err; + return ret; } /* Build up a fully qualified path for a config file to be @@ -1182,8 +1196,10 @@ int virFileWritePid(const char *dir, goto cleanup; } - if ((rc = virFileMakePath(dir))) + if (virFileMakePath(dir) < 0) { + rc = errno; goto cleanup; + } if (!(pidfile = virFilePid(dir, name))) { rc = ENOMEM; -- 1.7.4.1
On 07/05/2011 03:04 PM, Matthias Bolte wrote:
Some callers expected virFileMakePath to set errno, some expected it to return an errno value. Unify this to return 0 on success and -1 on error. Set errno to report detailed error information.
Also Make virFileMakePath report an error when stat fails with an errno different from ENOENT.
When I first read this, I thought that you meant that virFileMakePath would use virReportSystemError, but only on that particular failure path. But on reading the patch, I see that you merely meant that this patch now guarantees that virFileMakePath returns -1 for errno different than ENOENT right away, rather than wasting time calling strdup and eventually mkdir and possibly having the errno from mkdir be less specific than the errno from the initial failed stat. So maybe reword this as: Also optimize virFileMakePath if stat fails with an errno different from ENOENT.
@@ -429,8 +429,8 @@ static int lxcContainerMountBasicFS(virDomainFSDefPtr root) } }
- if ((rc = virFileMakePath("/dev/pts") != 0)) { - virReportSystemError(rc, "%s", + if (virFileMakePath("/dev/pts") < 0) { + virReportSystemError(errno, "%s",
Nice bug fix, by the way. Matthias had to point out to me on IRC that the parenthesis were wrong, and that virFileMakePAth("/dev/pts") != 0 ended up making rc only 0 or 1, rather than 0 or an errno value, making for a misleading virReportSystemError output pre-patch.
+++ b/src/util/util.c @@ -1010,66 +1010,80 @@ int virDirCreate(const char *path ATTRIBUTE_UNUSED, } #endif /* WIN32 */
-static int virFileMakePathHelper(char *path) { +static int virFileMakePathHelper(char *path) +{ struct stat st; char *p = NULL; - int err;
if (stat(path, &st) >= 0) return 0; + else if (errno != ENOENT) + return -1;
The 'else' isn't strictly necessary. And as long as we are taking shortcuts, should we use this instead? if (stat(path, &st) >= 0) { if (S_ISDIR(st.st_mode)) return 0; errno = ENOTDIR; return -1; } if (errno != ENOENT) return -1;
+/** + * Creates the given path with mode 0777 if it's not already existing
I probably would do s/path/directory/, although it's not critical to this patch.
+ * completely. + * + * Returns 0 on success, or -1 if an error occurred (in which case, errno + * is set appropriately). + */ int virFileMakePath(const char *path) { + int ret = -1; struct stat st; char *parent = NULL; char *p; - int err = 0;
if (stat(path, &st) >= 0) + return 0; + else if (errno != ENOENT) goto cleanup;
Same shortcut question as for virFileMakePathHelper.
if ((parent = strdup(path)) == NULL) { - err = ENOMEM; + errno = ENOMEM; goto cleanup; }
At this point, why don't we just call virFileMakePathHelper, rather than...
if ((p = strrchr(parent, '/')) == NULL) { - err = EINVAL; + errno = EINVAL; goto cleanup; }
if (p != parent) { *p = '\0'; - if ((err = virFileMakePathHelper(parent)) != 0) { + + if (virFileMakePathHelper(parent) < 0) goto cleanup; - } }
- if (mkdir(path, 0777) < 0 && errno != EEXIST) { - err = errno; + if (mkdir(path, 0777) < 0 && errno != EEXIST) goto cleanup; - }
...copying the same code for the strrchr, recursion, and mkdir ourselves? For that matter, even the initial stat() could be skipped, and have this function be _just_ the strdup and call into the recursive helper. On the other hand, that could be a cleanup for a separate patch; what you have here is minimally invasive for fixing the problem at hand, so: ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
2011/7/6 Eric Blake <eblake@redhat.com>:
On 07/05/2011 03:04 PM, Matthias Bolte wrote:
Some callers expected virFileMakePath to set errno, some expected it to return an errno value. Unify this to return 0 on success and -1 on error. Set errno to report detailed error information.
Also Make virFileMakePath report an error when stat fails with an errno different from ENOENT.
When I first read this, I thought that you meant that virFileMakePath would use virReportSystemError, but only on that particular failure path. But on reading the patch, I see that you merely meant that this patch now guarantees that virFileMakePath returns -1 for errno different than ENOENT right away, rather than wasting time calling strdup and eventually mkdir and possibly having the errno from mkdir be less specific than the errno from the initial failed stat. So maybe reword this as:
Also optimize virFileMakePath if stat fails with an errno different from ENOENT.
Yes, the word "report" gives the wrong impression, I'll go with your suggestion.
@@ -429,8 +429,8 @@ static int lxcContainerMountBasicFS(virDomainFSDefPtr root) } }
- if ((rc = virFileMakePath("/dev/pts") != 0)) { - virReportSystemError(rc, "%s", + if (virFileMakePath("/dev/pts") < 0) { + virReportSystemError(errno, "%s",
Nice bug fix, by the way. Matthias had to point out to me on IRC that the parenthesis were wrong, and that virFileMakePAth("/dev/pts") != 0 ended up making rc only 0 or 1, rather than 0 or an errno value, making for a misleading virReportSystemError output pre-patch.
This one was hard to spot. I didn't see it in the v1 of this patch that already touched this line. I only noticed it on the second visit.
+++ b/src/util/util.c @@ -1010,66 +1010,80 @@ int virDirCreate(const char *path ATTRIBUTE_UNUSED, } #endif /* WIN32 */
-static int virFileMakePathHelper(char *path) { +static int virFileMakePathHelper(char *path) +{ struct stat st; char *p = NULL; - int err;
if (stat(path, &st) >= 0) return 0; + else if (errno != ENOENT) + return -1;
The 'else' isn't strictly necessary. And as long as we are taking shortcuts, should we use this instead?
if (stat(path, &st) >= 0) { if (S_ISDIR(st.st_mode)) return 0; errno = ENOTDIR; return -1; } if (errno != ENOENT) return -1;
+/** + * Creates the given path with mode 0777 if it's not already existing
I probably would do s/path/directory/, although it's not critical to this patch.
I'll change that for clarity.
+ * completely. + * + * Returns 0 on success, or -1 if an error occurred (in which case, errno + * is set appropriately). + */ int virFileMakePath(const char *path) { + int ret = -1; struct stat st; char *parent = NULL; char *p; - int err = 0;
if (stat(path, &st) >= 0) + return 0; + else if (errno != ENOENT) goto cleanup;
Same shortcut question as for virFileMakePathHelper.
if ((parent = strdup(path)) == NULL) { - err = ENOMEM; + errno = ENOMEM; goto cleanup; }
At this point, why don't we just call virFileMakePathHelper, rather than...
if ((p = strrchr(parent, '/')) == NULL) { - err = EINVAL; + errno = EINVAL; goto cleanup; }
if (p != parent) { *p = '\0'; - if ((err = virFileMakePathHelper(parent)) != 0) { + + if (virFileMakePathHelper(parent) < 0) goto cleanup; - } }
- if (mkdir(path, 0777) < 0 && errno != EEXIST) { - err = errno; + if (mkdir(path, 0777) < 0 && errno != EEXIST) goto cleanup; - }
...copying the same code for the strrchr, recursion, and mkdir ourselves? For that matter, even the initial stat() could be skipped, and have this function be _just_ the strdup and call into the recursive helper.
On the other hand, that could be a cleanup for a separate patch; what you have here is minimally invasive for fixing the problem at hand, so:
ACK.
Thanks. I'll push this patch with an improved commit message and function comment and defer the logic improvements to a followup patch. -- Matthias Bolte http://photron.blogspot.com
participants (2)
-
Eric Blake -
Matthias Bolte