[Xen-devel] Re: [PATCH] Xenstore minor cleanup

[Rusty Russell <rusty@xxxxxxxxxxxxxxx>]

On Fri, 2005-06-24 at 15:57 +1000, Rusty Russell wrote:
> Update TODO list
> Wrap opendir in talloc so it gets cleaned up on OOM.
> Remove last call to system by open-coding "cp -al" to create transaction.
> 
> Signed-off-by: Rusty Russell <rusty@xxxxxxxxxxxxxxx>

Oops, disabling EROFS outside transaction snuck in.  Here's the right
patch:

Update TODO list
Wrap opendir in talloc so it gets cleaned up on OOM.
Remove last call to system by open-coding "cp -al" to create transaction.

Signed-off-by: Rusty Russell <rusty@xxxxxxxxxxxxxxx>
Index: TODO
===================================================================
RCS file: /var/cvs/xeno-unstable/tools/xenstore/TODO,v
retrieving revision 1.2
retrieving revision 1.3
diff -u -r1.2 -r1.3
--- TODO        18 May 2005 06:29:53 -0000      1.2
+++ TODO        23 Jun 2005 08:23:33 -0000      1.3
@@ -4,4 +4,6 @@
 
 - Remove calls to system() from daemon
 - Timeout failed watch responses
-- Timeout blocking transactions
+- Dynamic nodes
+- Persistant storage of introductions, watches and transactions, so daemon can 
restart
+- Remove assumption that rename doesn't fail
Index: xenstored_core.c
===================================================================
RCS file: /var/cvs/xeno-unstable/tools/xenstore/xenstored_core.c,v
retrieving revision 1.3
retrieving revision 1.6
diff -u -r1.3 -r1.6
--- xenstored_core.c    21 Jun 2005 12:00:32 -0000      1.3
+++ xenstored_core.c    24 Jun 2005 03:54:07 -0000      1.6
@@ -246,7 +246,7 @@
 }
 
 /* Read everything from a talloc_open'ed fd. */
-static void *read_all(int *fd, unsigned int *size)
+void *read_all(int *fd, unsigned int *size)
 {
        unsigned int max = 4;
        int ret;
@@ -271,7 +271,7 @@
 }
 
 /* Return a pointer to an fd, self-closing and attached to this pathname. */
-static int *talloc_open(const char *pathname, int flags, int mode)
+int *talloc_open(const char *pathname, int flags, int mode)
 {
        int *fd;
 
@@ -534,6 +534,30 @@
        return tmppath;
 }
 
+static int destroy_opendir(void *_dir)
+{
+       DIR **dir = _dir;
+       closedir(*dir);
+       return 0;
+}
+
+/* Return a pointer to a DIR*, self-closing and attached to this pathname. */
+DIR **talloc_opendir(const char *pathname)
+{
+       DIR **dir;
+
+       dir = talloc(pathname, DIR *);
+       *dir = opendir(pathname);
+       if (!*dir) {
+               int saved_errno = errno;
+               talloc_free(dir);
+               errno = saved_errno;
+               return NULL;
+       }
+       talloc_set_destructor(dir, destroy_opendir);
+       return dir;
+}
+
 /* We assume rename() doesn't fail on moves in same dir. */
 static void commit_tempfile(const char *path)
 {
@@ -677,7 +701,7 @@
 {
        char *path, *reply = talloc_strdup(node, "");
        unsigned int reply_len = 0;
-       DIR *dir;
+       DIR **dir;
        struct dirent *dirent;
 
        node = canonicalize(conn, node);
@@ -685,11 +709,11 @@
                return send_error(conn, errno);
 
        path = node_dir(conn->transaction, node);
-       dir = opendir(path);
+       dir = talloc_opendir(path);
        if (!dir)
                return send_error(conn, errno);
 
-       while ((dirent = readdir(dir)) != NULL) {
+       while ((dirent = readdir(*dir)) != NULL) {
                int len = strlen(dirent->d_name) + 1;
 
                if (!valid_chars(dirent->d_name))
@@ -699,7 +723,6 @@
                strcpy(reply + reply_len, dirent->d_name);
                reply_len += len;
        }
-       closedir(dir);
 
        return send_reply(conn, XS_DIRECTORY, reply, reply_len);
 }
Index: xenstored_core.h
===================================================================
RCS file: /var/cvs/xeno-unstable/tools/xenstore/xenstored_core.h,v
retrieving revision 1.2
retrieving revision 1.3
diff -u -r1.2 -r1.3
--- xenstored_core.h    20 Jun 2005 06:42:24 -0000      1.2
+++ xenstored_core.h    24 Jun 2005 03:54:07 -0000      1.3
@@ -20,6 +20,8 @@
 #ifndef _XENSTORED_CORE_H
 #define _XENSTORED_CORE_H
 
+#include <sys/types.h>
+#include <dirent.h>
 #include <stdbool.h>
 #include <stdint.h>
 #include <errno.h>
@@ -129,7 +131,16 @@
 /* Is this a valid node name? */
 bool is_valid_nodename(const char *node);
 
+/* Return a pointer to an open dir, self-closig and attached to pathname. */
+DIR **talloc_opendir(const char *pathname);
+
+/* Return a pointer to an fd, self-closing and attached to this pathname. */
+int *talloc_open(const char *pathname, int flags, int mode);
+
 /* Convenient talloc-style destructor for paths. */
 int destroy_path(void *path);
 
+/* Read entire contents of a talloced fd. */
+void *read_all(int *fd, unsigned int *size);
+
 #endif /* _XENSTORED_CORE_H */
Index: xenstored_transaction.c
===================================================================
RCS file: /var/cvs/xeno-unstable/tools/xenstore/xenstored_transaction.c,v
retrieving revision 1.1
retrieving revision 1.2
diff -u -r1.1 -r1.2
--- xenstored_transaction.c     20 Jun 2005 04:38:54 -0000      1.1
+++ xenstored_transaction.c     24 Jun 2005 03:54:07 -0000      1.2
@@ -25,6 +25,7 @@
 #include <time.h>
 #include <stdarg.h>
 #include <stdlib.h>
+#include <fcntl.h>
 #include "talloc.h"
 #include "list.h"
 #include "xenstored_transaction.h"
@@ -137,7 +138,7 @@
 
 char *node_dir_inside_transaction(struct transaction *trans, const char *node)
 {
-       return talloc_asprintf(node, "%s%s", trans->divert,
+       return talloc_asprintf(node, "%s/%s", trans->divert,
                               node + strlen(trans->node));
 }
 
@@ -170,33 +171,74 @@
        }
 }
 
-/* FIXME: Eliminate all uses of this */
-static bool do_command(const char *cmd)
+static int destroy_transaction(void *_transaction)
+{
+       struct transaction *trans = _transaction;
+
+       list_del(&trans->list);
+       return destroy_path(trans->divert);
+}
+
+static bool copy_file(const char *src, const char *dst)
 {
-       int ret;
+       int *infd, *outfd;
+       void *data;
+       unsigned int size;
 
-       ret = system(cmd);
-       if (ret == -1)
+       infd = talloc_open(src, O_RDONLY, 0);
+       if (!infd)
                return false;
-       if (!WIFEXITED(ret) || WEXITSTATUS(ret) != 0) {
-               errno = EIO;
+       outfd = talloc_open(dst, O_WRONLY|O_CREAT|O_EXCL, 0640);
+       if (!outfd)
                return false;
-       }
-       return true;
+       data = read_all(infd, &size);
+       if (!data)
+               return false;
+       return xs_write_all(*outfd, data, size);
 }
 
-static int destroy_transaction(void *_transaction)
+static bool copy_dir(const char *src, const char *dst)
 {
-       struct transaction *trans = _transaction;
+       DIR **dir;
+       struct dirent *dirent;
 
-       list_del(&trans->list);
-       return destroy_path(trans->divert);
+       if (mkdir(dst, 0750) != 0)
+               return false;
+
+       dir = talloc_opendir(src);
+       if (!dir)
+               return false;
+
+       while ((dirent = readdir(*dir)) != NULL) {
+               struct stat st;
+               char *newsrc, *newdst;
+
+               if (streq(dirent->d_name, ".") || streq(dirent->d_name, ".."))
+                       continue;
+
+               newsrc = talloc_asprintf(src, "%s/%s", src, dirent->d_name);
+               newdst = talloc_asprintf(src, "%s/%s", dst, dirent->d_name);
+               if (stat(newsrc, &st) != 0)
+                       return false;
+               
+               if (S_ISDIR(st.st_mode)) {
+                       if (!copy_dir(newsrc, newdst))
+                               return false;
+               } else {
+                       if (!copy_file(newsrc, newdst))
+                               return false;
+               }
+               /* Free now so we don't run out of file descriptors */
+               talloc_free(newsrc);
+               talloc_free(newdst);
+       }
+       return true;
 }
 
 bool do_transaction_start(struct connection *conn, const char *node)
 {
        struct transaction *transaction;
-       char *dir, *cmd;
+       char *dir;
 
        if (conn->transaction)
                return send_error(conn, EBUSY);
@@ -213,14 +255,9 @@
        /* Attach transaction to node for autofree until it's complete */
        transaction = talloc(node, struct transaction);
        transaction->node = talloc_strdup(transaction, node);
-       transaction->divert = talloc_asprintf(transaction, "%s/%p/", 
+       transaction->divert = talloc_asprintf(transaction, "%s/%p", 
                                              xs_daemon_transactions(),
                                              transaction);
-       cmd = talloc_asprintf(node, "cp -a %s %s", dir, transaction->divert);
-       if (!do_command(cmd))
-               corrupt(conn, "Creating transaction %s", transaction->divert);
-
-       talloc_steal(conn, transaction);
        INIT_LIST_HEAD(&transaction->changes);
        transaction->conn = conn;
        timerclear(&transaction->timeout);
@@ -228,6 +265,11 @@
        list_add_tail(&transaction->list, &transactions);
        conn->transaction = transaction;
        talloc_set_destructor(transaction, destroy_transaction);
+
+       if (!copy_dir(dir, transaction->divert))
+               return send_error(conn, errno);
+
+       talloc_steal(conn, transaction);
        return send_ack(transaction->conn, XS_TRANSACTION_START);
 }
 

-- 
A bad analogy is like a leaky screwdriver -- Richard Braakman


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel