[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCHv4 1/2] util: refactor virFileOpenAs



On 02/03/2012 03:41 PM, Eric Blake wrote:
On 02/01/2012 11:36 PM, Laine Stump wrote:
virFileOpenAs previously would only try opening a file as the current
user, or as a different user, but wouldn't try both methods in a
single call. This made it cumbersome to use as a replacement for
open(2). Additionally, it had a lot of historical baggage that led to
it being difficult to understand.

This patch refactors virFileOpenAs in the following ways:
All current consumers of virFileOpenAs should retain their present
behavior (after a few minor changes to their setup code and
arguments).
Yep, sure looks like it.

---
  src/libxl/libxl_driver.c      |    4 +-
  src/qemu/qemu_driver.c        |    8 +-
  src/storage/storage_backend.c |   12 +-
  src/util/util.c               |  352 +++++++++++++++++++++++++++--------------
  src/util/util.h               |    6 +-
  5 files changed, 246 insertions(+), 136 deletions(-)
I think we've hit a winner for refactoring it into something that can be
further modified while investigating those modifications, making it more
powerful for new uses, and without breaking behavior in the refactor.

+/* virFileOpenForceOwnerMode() - an internal utility function called
+ * only by virFileOpenAs().  Sets the owner and mode of the file
+ * opened as "fd" if it's not correct AND the flags say it should be
+ * forced. */
  static int
-virFileOpenAsNoFork(const char *path, int openflags, mode_t mode,
-                    uid_t uid, gid_t gid, unsigned int flags)
+virFileOpenForceOwnerMode(const char *path, int fd, mode_t mode,
+                          uid_t uid, gid_t gid, unsigned int flags)
  {
-    if ((flags&  VIR_DIR_CREATE_FORCE_PERMS)
-&&  (chmod(path, mode)<  0)) {
+    if ((flags&  VIR_FILE_OPEN_FORCE_MODE)&&
+        ((mode&  (S_IRWXU|S_IRWXG|S_IRWXO)) !=
+         (st.st_mode&  (S_IRWXU|S_IRWXG|S_IRWXO)))&&
Technically, this limits us to a mask of 00777,

+        (fchmod(fd, mode)<  0)) {
          ret = -errno;
          virReportSystemError(errno,
                               _("cannot set mode of '%s' to %04o"),
while this error message implied that we could request a mode with mask
07777.  But since none of the callers are passing sticky bits, I think
we can adjust that later if we ever find a reason that we need bits from
07000 modified, and leave well enough alone in this patch.

+/* virFileOpenForked() - an internal utility function called only by
+ * virFileOpenAs(). It forks, then the child does setuid+setgid to
+ * given uid:gid and attempts to open the file, while the parent just
+ * calls recvfd to get the open fd back from the child. returns the
+ * fd, or -errno if there is an error. */
+static int
+virFileOpenForked(const char *path, int openflags, mode_t mode,
+                  uid_t uid, gid_t gid, unsigned int flags)
  {
      pid_t pid;
      forkRet = virFork(&pid);
-
-    if (pid<  0) {
-        ret = -errno;
-        return ret;
-    }
+    if (pid<  0)
+        return -errno;

      if (pid) { /* parent */
You know, the diff for this patch is so crazy already that we might as
well make maintenance slightly easier.  That is, right now, we have:

fork
if fail
     return error
if parent {
     wait for child
     do stuff, which has several return calls
     return
}
child
     do stuff, which might go to childerror
childerr:
     _exit

But sequentially, since the parent is waiting on the child, it might be
easier to read as:

fork
if child {
     do stuff, which might go to childerror
childerror:
     _exit
}
parent
     if fork failed, go to cleanup
     wait for child
     do more stuff, which might go to cleanup
cleanup:
     return

In other words, if you want to rearrange this section of code, now might
be a good time to do it, and I wouldn't even mind if you squashed it in
to this one rather than using a separate patch.  But that's all
cosmetic, it wouldn't change the code you actually have.


          if (!WIFEXITED(status) || (ret = -WEXITSTATUS(status)) == -EACCES ||
              fd == -1) {
              /* fall back to the simpler method, which works better in
               * some cases */
              VIR_FORCE_CLOSE(fd);
-            flags&= ~VIR_FILE_OPEN_AS_UID;
-            return virFileOpenAsNoFork(path, openflags, mode, uid, gid, flags);
+            if (flags&  VIR_FILE_OPEN_NOFORK) {
+                /* If we had already tried opening w/o fork+setuid and
+                 * failed, no sense trying again. Just set return the
+                 * original errno that we got at that time (by
+                 * definition, always either EACCES or EPERM - EACCES
+                 * is close enough).
+                 */
+               return -EACCES;
Fair enough.  I don't think the slight loss in information will hurt;
the end result is still a reasonable failure message.

ACK.


reordered and pushed. Thanks!


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]