[libvirt] [PATCHv4 09/15] util: rename virFileOperation to virFileOpenAs
Laine Stump
laine at laine.org
Fri Mar 11 15:39:38 UTC 2011
On 03/09/2011 08:45 PM, Eric Blake wrote:
> This patch intentionally doesn't change indentation, in order to
> make it easier to review the real changes.
>
> * src/util/util.h (VIR_FILE_OP_RETURN_FD, virFileOperationHook):
> Delete.
> (virFileOperation): Rename...
> (virFileOpenAs): ...and reduce parameters.
> * src/util/util.c (virFileOperationNoFork, virFileOperation):
> Rename and simplify.
> * src/qemu/qemu_driver.c (qemudDomainSaveFlag): Adjust caller.
> * src/storage/storage_backend.c (virStorageBackendCreateRaw):
> Likewise.
> * src/libvirt_private.syms: Reflect rename.
> ---
> src/libvirt_private.syms | 2 +-
> src/qemu/qemu_driver.c | 20 +++-----
> src/storage/storage_backend.c | 12 ++--
> src/util/util.c | 103 +++++++++++++----------------------------
> src/util/util.h | 15 ++----
> 5 files changed, 53 insertions(+), 99 deletions(-)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index c0da78e..a9f7e39 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -876,8 +876,8 @@ virFileIsExecutable;
> virFileLinkPointsTo;
> virFileMakePath;
> virFileMatchesNameSuffix;
> +virFileOpenAs;
> virFileOpenTty;
> -virFileOperation;
> virFilePid;
> virFileReadAll;
> virFileReadLimFD;
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index a73c5b9..06bc969 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -1880,11 +1880,9 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver, virDomainPtr dom,
> goto endjob;
> }
> } else {
> - if ((fd = virFileOperation(path, O_CREAT|O_TRUNC|O_WRONLY,
> - S_IRUSR|S_IWUSR,
> - getuid(), getgid(),
> - NULL, NULL,
> - VIR_FILE_OP_RETURN_FD))< 0) {
> + if ((fd = virFileOpenAs(path, O_CREAT|O_TRUNC|O_WRONLY,
> + S_IRUSR|S_IWUSR,
> + getuid(), getgid(), 0))< 0) {
> /* If we failed as root, and the error was permission-denied
> (EACCES or EPERM), assume it's on a network-connected share
> where root access is restricted (eg, root-squashed NFS). If the
> @@ -1918,7 +1916,7 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver, virDomainPtr dom,
>
> case 0:
> default:
> - /* local file - log the error returned by virFileOperation */
> + /* local file - log the error returned by virFileOpenAs */
> virReportSystemError(-rc,
> _("Failed to create domain save file '%s'"),
> path);
> @@ -1929,12 +1927,10 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver, virDomainPtr dom,
>
> /* Retry creating the file as driver->user */
>
> - if ((fd = virFileOperation(path, O_CREAT|O_TRUNC|O_WRONLY,
> - S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP,
> - driver->user, driver->group,
> - NULL, NULL,
> - (VIR_FILE_OP_AS_UID |
> - VIR_FILE_OP_RETURN_FD)))< 0) {
> + if ((fd = virFileOpenAs(path, O_CREAT|O_TRUNC|O_WRONLY,
> + S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP,
> + driver->user, driver->group,
> + VIR_FILE_OPEN_AS_UID))< 0) {
> virReportSystemError(-fd,
> _("Error from child process creating '%s'"),
> path);
> diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
> index c7c5ccd..7a3a2b8 100644
> --- a/src/storage/storage_backend.c
> +++ b/src/storage/storage_backend.c
> @@ -364,14 +364,14 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED,
>
> uid = (vol->target.perms.uid == -1) ? getuid() : vol->target.perms.uid;
> gid = (vol->target.perms.gid == -1) ? getgid() : vol->target.perms.gid;
> - operation_flags = VIR_FILE_OP_FORCE_PERMS | VIR_FILE_OP_RETURN_FD;
> + operation_flags = VIR_FILE_OPEN_FORCE_PERMS;
> if (pool->def->type == VIR_STORAGE_POOL_NETFS)
> - operation_flags |= VIR_FILE_OP_AS_UID;
> + operation_flags |= VIR_FILE_OPEN_AS_UID;
>
> - if ((fd = virFileOperation(vol->target.path,
> - O_RDWR | O_CREAT | O_EXCL,
> - vol->target.perms.mode, uid, gid,
> - NULL, NULL, operation_flags))< 0) {
> + if ((fd = virFileOpenAs(vol->target.path,
> + O_RDWR | O_CREAT | O_EXCL,
> + vol->target.perms.mode, uid, gid,
> + operation_flags))< 0) {
> virReportSystemError(-fd,
> _("cannot create path '%s'"),
> vol->target.path);
> diff --git a/src/util/util.c b/src/util/util.c
> index 6bf3219..137cdb9 100644
> --- a/src/util/util.c
> +++ b/src/util/util.c
> @@ -1355,10 +1355,10 @@ virFileIsExecutable(const char *file)
>
> #ifndef WIN32
> /* return -errno on failure, or 0 on success */
> -static int virFileOperationNoFork(const char *path, int openflags, mode_t mode,
> - uid_t uid, gid_t gid,
> - virFileOperationHook hook, void *hookdata,
> - unsigned int flags) {
> +static int
> +virFileOpenAsNoFork(const char *path, int openflags, mode_t mode,
> + uid_t uid, gid_t gid, unsigned int flags)
> +{
> int fd = -1;
> int ret = 0;
> struct stat st;
> @@ -1381,7 +1381,7 @@ static int virFileOperationNoFork(const char *path, int openflags, mode_t mode,
> path, (unsigned int) uid, (unsigned int) gid);
> goto error;
> }
> - if ((flags& VIR_FILE_OP_FORCE_PERMS)
> + if ((flags& VIR_FILE_OPEN_FORCE_PERMS)
> && (fchmod(fd, mode)< 0)) {
> ret = -errno;
> virReportSystemError(errno,
> @@ -1389,17 +1389,7 @@ static int virFileOperationNoFork(const char *path, int openflags, mode_t mode,
> path, mode);
> goto error;
> }
> - if ((hook)&& ((ret = hook(fd, hookdata)) != 0)) {
> - goto error;
> - }
> - if (flags& VIR_FILE_OP_RETURN_FD)
> - return fd;
> - if (VIR_CLOSE(fd)< 0) {
> - ret = -errno;
> - virReportSystemError(errno, _("failed to close new file '%s'"),
> - path);
> - goto error;
> - }
> + return fd;
>
> error:
> VIR_FORCE_CLOSE(fd);
> @@ -1446,35 +1436,27 @@ error:
> }
>
> /**
> - * virFileOperation:
> + * virFileOpenAs:
> * @path: file to open or create
> * @openflags: flags to pass to open
> * @mode: mode to use on creation or when forcing permissions
> * @uid: uid that should own file on creation
> * @gid: gid that should own file
> - * @hook: callback to run once file is open; see below for restrictions
> - * @hookdata: opaque data for @hook
> - * @flags: bit-wise or of VIR_FILE_OP_* flags
> + * @flags: bit-wise or of VIR_FILE_OPEN_* flags
> *
> * Open @path, and execute an optional callback @hook on the open file
> * description. @hook must return 0 on success, or -errno on failure.
> - * If @flags includes VIR_FILE_OP_AS_UID, then open the file while the
> + * If @flags includes VIR_FILE_OPEN_AS_UID, then open the file while the
> * effective user id is @uid (by using a child process); this allows
> * one to bypass root-squashing NFS issues. If @flags includes
> - * VIR_FILE_OP_FORCE_PERMS, then ensure that @path has those
> + * VIR_FILE_OPEN_FORCE_PERMS, then ensure that @path has those
> * permissions before the callback, even if the file already existed
> - * with different permissions. If @flags includes
> - * VIR_FILE_OP_RETURN_FD, then use SCM_RIGHTS to guarantee that any
> - * @hook is run in the parent, and the return value (if non-negative)
> - * is the file descriptor, left open. Otherwise, @hook might be run
> - * in a child process, so it must be async-signal-safe (ie. no
> - * malloc() or anything else that depends on modifying locks held in
> - * the parent), no file descriptor remains open, and 0 is returned on
> - * success. Return -errno on failure. */
> -int virFileOperation(const char *path, int openflags, mode_t mode,
> - uid_t uid, gid_t gid,
> - virFileOperationHook hook, void *hookdata,
> - unsigned int flags) {
> + * with different permissions. The return value (if non-negative)
> + * is the file descriptor, left open. Return -errno on failure. */
> +int
> +virFileOpenAs(const char *path, int openflags, mode_t mode,
> + uid_t uid, gid_t gid, unsigned int flags)
> +{
> struct stat st;
> pid_t pid;
> int waitret, status, ret = 0;
> @@ -1487,11 +1469,10 @@ int virFileOperation(const char *path, int openflags, mode_t mode,
> struct iovec iov;
> int forkRet;
>
> - if ((!(flags& VIR_FILE_OP_AS_UID))
> + if ((!(flags& VIR_FILE_OPEN_AS_UID))
> || (getuid() != 0)
> || ((uid == 0)&& (gid == 0))) {
> - return virFileOperationNoFork(path, openflags, mode, uid, gid,
> - hook, hookdata, flags);
> + return virFileOpenAsNoFork(path, openflags, mode, uid, gid, flags);
> }
>
> /* parent is running as root, but caller requested that the
> @@ -1499,7 +1480,7 @@ int virFileOperation(const char *path, int openflags, mode_t mode,
> * following dance avoids problems caused by root-squashing
> * NFS servers. */
>
> - if (flags& VIR_FILE_OP_RETURN_FD) {
> + {
> if (socketpair(AF_UNIX, SOCK_DGRAM, 0, pair)< 0) {
> ret = -errno;
> virReportSystemError(errno,
> @@ -1529,7 +1510,7 @@ int virFileOperation(const char *path, int openflags, mode_t mode,
> }
>
> if (pid) { /* parent */
> - if (flags& VIR_FILE_OP_RETURN_FD) {
> + {
> VIR_FORCE_CLOSE(pair[1]);
>
> do {
> @@ -1565,20 +1546,13 @@ int virFileOperation(const char *path, int openflags, mode_t mode,
> goto parenterror;
> }
> ret = -WEXITSTATUS(status);
> - if (!WIFEXITED(status) || (ret == -EACCES) ||
> - ((flags& VIR_FILE_OP_RETURN_FD)&& fd == -1)) {
> + if (!WIFEXITED(status) || (ret == -EACCES) || fd == -1) {
> /* fall back to the simpler method, which works better in
> * some cases */
> - return virFileOperationNoFork(path, openflags, mode, uid, gid,
> - hook, hookdata, flags);
> + return virFileOpenAsNoFork(path, openflags, mode, uid, gid, flags);
> }
> - if (!ret&& (flags& VIR_FILE_OP_RETURN_FD)) {
> - if ((hook)&& ((ret = hook(fd, hookdata)) != 0)) {
> - VIR_FORCE_CLOSE(fd);
> - goto parenterror;
> - }
> + if (!ret)
> ret = fd;
> - }
> parenterror:
> return ret;
> }
> @@ -1630,7 +1604,7 @@ parenterror:
> path, (unsigned int) uid, (unsigned int) gid);
> goto childerror;
> }
> - if ((flags& VIR_FILE_OP_FORCE_PERMS)
> + if ((flags& VIR_FILE_OPEN_FORCE_PERMS)
> && (fchmod(fd, mode)< 0)) {
> ret = -errno;
> virReportSystemError(errno,
> @@ -1638,7 +1612,7 @@ parenterror:
> path, mode);
> goto childerror;
> }
> - if (flags& VIR_FILE_OP_RETURN_FD) {
> + {
> VIR_FORCE_CLOSE(pair[0]);
This VIR_FORCE_CLOSE(pair[0]) should be moved up to the very start of
the child code - otherwise any error prior to this point will leave it
dangling for _exit() to clean up.
> memcpy(CMSG_DATA(cmsg),&fd, sizeof(fd));
>
> @@ -1651,17 +1625,6 @@ parenterror:
> VIR_FORCE_CLOSE(pair[1]);
A leftover from 05/15 - I think thie VIR_FORCE_CLOSE should happen
at/after childerror:. That way it will always get cleaned up, whether or
not there is an error at any time during child execution.
> goto childerror;
> }
> - ret = 0;
> - } else {
> - if ((hook)&& ((ret = hook(fd, hookdata)) != 0))
> - goto childerror;
> - if (VIR_CLOSE(fd)< 0) {
> - ret = -errno;
> - virReportSystemError(errno,
> - _("child failed to close new file '%s'"),
> - path);
> - goto childerror;
> - }
> }
>
> ret = 0;
> @@ -1783,17 +1746,15 @@ childerror:
> #else /* WIN32 */
>
> /* return -errno on failure, or 0 on success */
> -int virFileOperation(const char *path ATTRIBUTE_UNUSED,
> - int openflags ATTRIBUTE_UNUSED,
> - mode_t mode ATTRIBUTE_UNUSED,
> - uid_t uid ATTRIBUTE_UNUSED,
> - gid_t gid ATTRIBUTE_UNUSED,
> - virFileOperationHook hook ATTRIBUTE_UNUSED,
> - void *hookdata ATTRIBUTE_UNUSED,
> - unsigned int flags ATTRIBUTE_UNUSED)
> +int virFileOpenAs(const char *path ATTRIBUTE_UNUSED,
> + int openflags ATTRIBUTE_UNUSED,
> + mode_t mode ATTRIBUTE_UNUSED,
> + uid_t uid ATTRIBUTE_UNUSED,
> + gid_t gid ATTRIBUTE_UNUSED,
> + unsigned int flags ATTRIBUTE_UNUSED)
> {
> virUtilError(VIR_ERR_INTERNAL_ERROR,
> - "%s", _("virFileOperation is not implemented for WIN32"));
> + "%s", _("virFileOpenAs is not implemented for WIN32"));
>
> return -ENOSYS;
> }
> diff --git a/src/util/util.h b/src/util/util.h
> index 3970d58..cecd348 100644
> --- a/src/util/util.h
> +++ b/src/util/util.h
> @@ -125,16 +125,13 @@ bool virFileIsExecutable(const char *file) ATTRIBUTE_NONNULL(1);
> char *virFileSanitizePath(const char *path);
>
> enum {
> - VIR_FILE_OP_NONE = 0,
> - VIR_FILE_OP_AS_UID = (1<< 0),
> - VIR_FILE_OP_FORCE_PERMS = (1<< 1),
> - VIR_FILE_OP_RETURN_FD = (1<< 2),
> + VIR_FILE_OPEN_NONE = 0,
> + VIR_FILE_OPEN_AS_UID = (1<< 0),
> + VIR_FILE_OPEN_FORCE_PERMS = (1<< 1),
> };
> -typedef int (*virFileOperationHook)(int fd, void *data);
> -int virFileOperation(const char *path, int openflags, mode_t mode,
> - uid_t uid, gid_t gid,
> - virFileOperationHook hook, void *hookdata,
> - unsigned int flags)
> +int virFileOpenAs(const char *path, int openflags, mode_t mode,
> + uid_t uid, gid_t gid,
> + unsigned int flags)
> ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
>
> enum {
Aside from the two problems with the locations of
VIR_FORCE_CLOSE(pair[n]), this all looks fine, and hasn't broken either
backing store or save images on root-squash NFS, so ACK with those two
changes.
More information about the libvir-list
mailing list