[Qemu-devel] [PATCH v2 0/2] virtagent - fsfreeze support

[Qemu-devel] [PATCH v2 0/2] virtagent - fsfreeze support

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Hi

This is a first attempt to add fsfreeze support to virtagent. The idea
is for the guest agent to walk the list of locally mounted file
systems in the guest, and issuing an ioctl to freeze them. The host
can then do a live snapshot of the guest, obtaining stable file
systems. After the snapshot, the host then calls the thaw function in
virtagent, which goes through the list of previously frozen file
systems and unfreezes them.

The list walking ignores remote file systems such as NFS and CIFS as
well as all pseudo file systems.

The guest agent code is in the first patch, and host agent code is in
the second patch. For now there is only human monitor support, but it
should be pretty straight forward to add QMP support as well.

Comments and suggestions welcome!

v2 of the patch addresses the issues pointed out by Stefan and Michael.

Cheers,
Jes


Jes Sorensen (2):
  Add virtagent file system freeze/thaw
  Add monitor commands for fsfreeze support

 hmp-commands.hx    |   48 +++++++++++
 virtagent-common.h |    9 ++
 virtagent-server.c |  190 ++++++++++++++++++++++++++++++++++++++++++
 virtagent.c        |  235 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 virtagent.h        |    9 ++
 5 files changed, 491 insertions(+), 0 deletions(-)

-- 
1.7.3.5

[Qemu-devel] [PATCH 1/2] Add virtagent file system freeze/thaw

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Implement freeze/thaw support in the guest, allowing the host to
request the guest freezes all it's file systems before a live snapshot
is performed.
 - fsfreeze(): Walk the list of mounted local real file systems,
               and freeze them.
 - fsthaw():   Walk the list of previously frozen file systems and
               thaw them.
 - fsstatus(): Return the current status of freeze/thaw. The host must
               poll this function, in case fsfreeze() returned with a
	       timeout, to wait for the operation to finish.

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 virtagent-common.h |    8 ++
 virtagent-server.c |  190 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 198 insertions(+), 0 deletions(-)

diff --git a/virtagent-common.h b/virtagent-common.h
index 5d8f5c1..7c6d9ef 100644
--- a/virtagent-common.h
+++ b/virtagent-common.h
@@ -61,6 +61,14 @@ typedef struct VAContext {
     const char *channel_path;
 } VAContext;
 
+enum va_fsfreeze_status {
+    FREEZE_ERROR = -1,
+    FREEZE_THAWED = 0,
+    FREEZE_INPROGRESS = 1,
+    FREEZE_FROZEN = 2,
+    FREEZE_THAWINPROGRESS = 3,
+};
+
 enum va_job_status {
     VA_JOB_STATUS_PENDING = 0,
     VA_JOB_STATUS_OK,
diff --git a/virtagent-server.c b/virtagent-server.c
index 7bb35b2..ffbe163 100644
--- a/virtagent-server.c
+++ b/virtagent-server.c
@@ -14,6 +14,10 @@
 #include <syslog.h>
 #include "qemu_socket.h"
 #include "virtagent-common.h"
+#include <mntent.h>
+#include <sys/types.h>
+#include <sys/ioctl.h>
+#include <linux/fs.h>
 
 static VAServerData *va_server_data;
 static bool va_enable_syslog = false; /* enable syslog'ing of RPCs */
@@ -217,6 +221,186 @@ static xmlrpc_value *va_hello(xmlrpc_env *env,
     return result;
 }
 
+
+/*
+ * Walk the mount table and build a list of local file systems
+ */
+
+struct direntry {
+    char *dirname;
+    char *devtype;
+    struct direntry *next;
+};
+
+static struct direntry *va_mount_list;
+static int va_fsfreeze_status;
+
+static int build_mount_list(void)
+{
+    struct mntent *mnt;
+    struct direntry *entry;
+    struct direntry *next;
+    char const *mtab = MOUNTED;
+    FILE *fp;
+
+    fp = setmntent(mtab, "r");
+    if (!fp) {
+	fprintf(stderr, "unable to read mtab\n");
+	goto fail;
+    }
+
+    while ((mnt = getmntent(fp))) {
+	/*
+	 * An entry which device name doesn't start with a '/' is
+	 * either a dummy file system or a network file system.
+	 * Add special handling for smbfs and cifs as is done by
+	 * coreutils as well.
+	 */
+	if ((mnt->mnt_fsname[0] != '/') ||
+	    (strcmp(mnt->mnt_type, "smbfs") == 0) ||
+	    (strcmp(mnt->mnt_type, "cifs") == 0)) {
+	    continue;
+	}
+
+	entry = qemu_malloc(sizeof(struct direntry));
+	entry->dirname = qemu_strdup(mnt->mnt_dir);
+	entry->devtype = qemu_strdup(mnt->mnt_type);
+	entry->next = va_mount_list;
+
+	va_mount_list = entry;
+    }
+
+    endmntent(fp);
+
+    return 0;
+ 
+fail:
+    while(va_mount_list) {
+	next = va_mount_list->next;
+        qemu_free(va_mount_list->dirname);
+        qemu_free(va_mount_list->devtype);
+        qemu_free(va_mount_list);
+        va_mount_list = next;
+    }
+
+    return -1;
+}
+
+/*
+ * va_fsfreeze(): Walk list of mounted file systems in the guest, and
+ *   freeze the ones which are real local file systems.
+ * rpc return values: Number of file systems frozen, -1 on error.
+ */
+static xmlrpc_value *va_fsfreeze(xmlrpc_env *env,
+                                 xmlrpc_value *params,
+                                 void *user_data)
+{
+    xmlrpc_int32 ret = 0, i = 0;
+    xmlrpc_value *result;
+    struct direntry *entry;
+    int fd;
+    SLOG("va_fsfreeze()");
+
+    if (va_fsfreeze_status != FREEZE_THAWED) {
+        ret = 0;
+        goto out;
+    }
+
+    ret = build_mount_list();
+    if (ret < 0) {
+        goto out;
+    }
+
+    va_fsfreeze_status = FREEZE_INPROGRESS;
+
+    entry = va_mount_list;
+    while(entry) {
+        fd = qemu_open(entry->dirname, O_RDONLY);
+        if (fd == -1) {
+            ret = errno;
+            goto error;
+        }
+        ret = ioctl(fd, FIFREEZE);
+        close(fd);
+        if (ret < 0 && ret != EOPNOTSUPP) {
+            goto error;
+        }
+
+        entry = entry->next;
+        i++;
+    }
+
+    va_fsfreeze_status = FREEZE_FROZEN;
+    ret = i;
+out:
+    result = xmlrpc_build_value(env, "i", ret);
+    return result;
+error:
+    if (i > 0) {
+        va_fsfreeze_status = FREEZE_ERROR;
+    }
+    goto out;
+}
+
+/*
+ * va_fsthaw(): Walk list of frozen file systems in the guest, and
+ *   thaw them.
+ * rpc return values: Number of file systems thawed on success, -1 on error.
+ */
+static xmlrpc_value *va_fsthaw(xmlrpc_env *env,
+                               xmlrpc_value *params,
+                               void *user_data)
+{
+    xmlrpc_int32 ret;
+    xmlrpc_value *result;
+    struct direntry *entry;
+    int fd, i = 0;
+    SLOG("va_fsthaw()");
+
+    if (va_fsfreeze_status != FREEZE_FROZEN) {
+        ret = 0;
+        goto out;
+    }
+
+    while((entry = va_mount_list)) {
+        fd = qemu_open(entry->dirname, O_RDONLY);
+        if (fd == -1) {
+            ret = -1;
+            goto out;
+        }
+        ret = ioctl(fd, FITHAW);
+        close(fd);
+        if (ret < 0 && ret != EOPNOTSUPP) {
+            ret = -1;
+            goto out;
+        }
+
+        va_mount_list = entry->next;
+        qemu_free(entry->dirname);
+        qemu_free(entry->devtype);
+        qemu_free(entry);
+        i++;
+    }
+
+    va_fsfreeze_status = FREEZE_THAWED;
+    ret = i;
+out:
+    result = xmlrpc_build_value(env, "i", ret);
+    return result;
+}
+
+/* va_fsstatus(): Return status of freeze/thaw
+ * rpc return values: fsfreeze_status
+ */
+static xmlrpc_value *va_fsstatus(xmlrpc_env *env,
+                                 xmlrpc_value *params,
+                                 void *user_data)
+{
+    xmlrpc_value *result = xmlrpc_build_value(env, "i", va_fsfreeze_status);
+    SLOG("va_fsstatus()");
+    return result;
+}
+
 typedef struct RPCFunction {
     xmlrpc_value *(*func)(xmlrpc_env *env, xmlrpc_value *param, void *unused);
     const char *func_name;
@@ -237,6 +421,12 @@ static RPCFunction guest_functions[] = {
       .func_name = "va.ping" },
     { .func = va_capabilities,
       .func_name = "va.capabilities" },
+    { .func = va_fsfreeze,
+      .func_name = "va.fsfreeze" },
+    { .func = va_fsthaw,
+      .func_name = "va.fsthaw" },
+    { .func = va_fsstatus,
+      .func_name = "va.fsstatus" },
     { NULL, NULL }
 };
 static RPCFunction host_functions[] = {
-- 
1.7.3.5

[Qemu-devel] [PATCH 2/2] Add monitor commands for fsfreeze support

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

This patch adds the following monitor commands:

agent_fsfreeze:
 - Freezes all local file systems in the guest. Command will print
   the number of file systems that were frozen.
agent_fsthaw:
 - Thaws all local file systems in the guest. Command will print
   the number of file systems that were thawed.
agent_fsstatus:
 - Prints the current status of file systems in the guest:
   Thawed, frozen, thaw in progress, freeze in progress, error.

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 hmp-commands.hx    |   48 +++++++++++
 virtagent-common.h |    1 +
 virtagent.c        |  235 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 virtagent.h        |    9 ++
 4 files changed, 293 insertions(+), 0 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 9c7ac0b..f4150da 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1310,6 +1310,54 @@ STEXI
 Fetch and re-negotiate guest agent capabilties
 ETEXI
 
+    {
+        .name       = "agent_fsfreeze",
+        .args_type  = "",
+        .params     = "",
+        .help       = "Freeze all local file systems mounted in the guest",
+        .user_print = do_agent_fsfreeze_print,
+        .mhandler.cmd_async = do_agent_fsfreeze,
+        .flags      = MONITOR_CMD_ASYNC,
+    },
+
+STEXI
+@item agent_fsfreeze
+@findex agent_fsfreeze
+Freeze all local mounted file systems in guest
+ETEXI
+
+    {
+        .name       = "agent_fsthaw",
+        .args_type  = "",
+        .params     = "",
+        .help       = "Thaw all local file systems mounted in the guest",
+        .user_print = do_agent_fsthaw_print,
+        .mhandler.cmd_async = do_agent_fsthaw,
+        .flags      = MONITOR_CMD_ASYNC,
+    },
+
+STEXI
+@item agent_fsthaw
+@findex agent_fsthaw
+Thaw all local mounted file systems in guest
+ETEXI
+
+    {
+        .name       = "agent_fsstatus",
+        .args_type  = "",
+        .params     = "",
+        .help       = "Display status of file system freeze progress in guest",
+        .user_print = do_agent_fsstatus_print,
+        .mhandler.cmd_async = do_agent_fsstatus,
+        .flags      = MONITOR_CMD_ASYNC,
+    },
+
+STEXI
+@item agent_fsstatus
+@findex agent_fsstatus
+Get status of file system freeze in guest
+ETEXI
+
 STEXI
 @end table
 ETEXI
diff --git a/virtagent-common.h b/virtagent-common.h
index 7c6d9ef..ff7bf23 100644
--- a/virtagent-common.h
+++ b/virtagent-common.h
@@ -24,6 +24,7 @@
 #include "monitor.h"
 #include "virtagent-server.h"
 #include "virtagent.h"
+#include "qint.h"
 
 #define DEBUG_VA
 
diff --git a/virtagent.c b/virtagent.c
index b5e7944..4277802 100644
--- a/virtagent.c
+++ b/virtagent.c
@@ -640,3 +640,238 @@ int va_send_hello(void)
     xmlrpc_DECREF(params);
     return ret;
 }
+
+void do_agent_fsfreeze_print(Monitor *mon, const QObject *data)
+{
+    TRACE("called");
+
+    monitor_printf(mon, "File systems frozen: %" PRId64 "\n",
+                   qint_get_int((qobject_to_qint(data))));
+}
+
+static void do_agent_fsfreeze_cb(const char *resp_data,
+                                 size_t resp_data_len,
+                                 MonitorCompletion *mon_cb,
+                                 void *mon_data)
+{
+    xmlrpc_value *resp = NULL;
+    xmlrpc_env env;
+    xmlrpc_int32 retval = 0;
+    QInt *qint;
+
+    TRACE("called");
+
+    if (resp_data == NULL) {
+        LOG("error handling RPC request");
+        return;
+    }
+
+    xmlrpc_env_init(&env);
+    resp = xmlrpc_parse_response(&env, resp_data, resp_data_len);
+    if (va_rpc_has_error(&env)) {
+        LOG("error parsing RPC response");
+        return;
+    }
+
+    xmlrpc_parse_value(&env, resp, "i", &retval);
+    if (va_rpc_has_error(&env)) {
+        retval = -1;
+        goto out;
+    }
+
+out:
+    qint = qint_from_int(retval);
+    xmlrpc_DECREF(resp);
+    if (mon_cb) {
+        mon_cb(mon_data, QOBJECT(qint));
+    }
+    qobject_decref(QOBJECT(qint));
+}
+
+int do_agent_fsfreeze(Monitor *mon, const QDict *mon_params,
+                      MonitorCompletion cb, void *opaque)
+{
+    xmlrpc_env env;
+    xmlrpc_value *params;
+    int ret;
+
+    TRACE("called");
+
+    xmlrpc_env_init(&env);
+    params = xmlrpc_build_value(&env, "()");
+    if (va_rpc_has_error(&env)) {
+        return -1;
+    }
+
+    ret = va_do_rpc(&env, "va.fsfreeze", params, do_agent_fsfreeze_cb,
+                    cb, opaque);
+    if (ret) {
+        qerror_report(QERR_VA_FAILED, ret, strerror(ret));
+    }
+    xmlrpc_DECREF(params);
+    return ret;
+}
+
+void do_agent_fsthaw_print(Monitor *mon, const QObject *data)
+{
+    TRACE("called");
+
+    monitor_printf(mon, "File systems thawed: %" PRId64 "\n",
+                   qint_get_int((qobject_to_qint(data))));
+}
+
+static void do_agent_fsthaw_cb(const char *resp_data,
+                               size_t resp_data_len,
+                               MonitorCompletion *mon_cb,
+                               void *mon_data)
+{
+    xmlrpc_value *resp = NULL;
+    xmlrpc_env env;
+    xmlrpc_int32 retval = 0;
+    QInt *qint;
+
+    TRACE("called");
+
+    if (resp_data == NULL) {
+        LOG("error handling RPC request");
+        return;
+    }
+
+    xmlrpc_env_init(&env);
+    resp = xmlrpc_parse_response(&env, resp_data, resp_data_len);
+    if (va_rpc_has_error(&env)) {
+        LOG("error parsing RPC response");
+        return;
+    }
+
+    xmlrpc_parse_value(&env, resp, "i", &retval);
+    if (va_rpc_has_error(&env)) {
+        retval = -1;
+        goto out;
+    }
+
+out:
+    qint = qint_from_int(retval);
+    xmlrpc_DECREF(resp);
+    if (mon_cb) {
+        mon_cb(mon_data, QOBJECT(qint));
+    }
+    qobject_decref(QOBJECT(qint));
+}
+
+int do_agent_fsthaw(Monitor *mon, const QDict *mon_params,
+                    MonitorCompletion cb, void *opaque)
+{
+    xmlrpc_env env;
+    xmlrpc_value *params;
+    int ret;
+
+    TRACE("called");
+
+    xmlrpc_env_init(&env);
+    params = xmlrpc_build_value(&env, "()");
+    if (va_rpc_has_error(&env)) {
+        return -1;
+    }
+
+    ret = va_do_rpc(&env, "va.fsthaw", params, do_agent_fsthaw_cb, cb, opaque);
+    if (ret) {
+        qerror_report(QERR_VA_FAILED, ret, strerror(ret));
+    }
+    xmlrpc_DECREF(params);
+    return ret;
+}
+
+void do_agent_fsstatus_print(Monitor *mon, const QObject *data)
+{
+    int64_t fsstatus;
+    TRACE("called");
+
+    fsstatus = qint_get_int((qobject_to_qint(data)));
+
+    monitor_printf(mon, "File systems freeze status: ");
+    switch(fsstatus) {
+    case FREEZE_THAWED:
+        monitor_printf(mon, "Thawed");
+        break;
+    case FREEZE_INPROGRESS:
+        monitor_printf(mon, "Freeze in progress");
+        break;
+    case FREEZE_THAWINPROGRESS:
+        monitor_printf(mon, "Thaw in progress");
+        break;
+    case FREEZE_FROZEN:
+        monitor_printf(mon, "Frozen");
+        break;
+    case FREEZE_ERROR:
+        monitor_printf(mon, "Error");
+        break;
+    default:
+        monitor_printf(mon, "unknown");
+    }
+
+    monitor_printf(mon, "\n");
+}
+
+static void do_agent_fsstatus_cb(const char *resp_data,
+                                 size_t resp_data_len,
+                                 MonitorCompletion *mon_cb,
+                                 void *mon_data)
+{
+    xmlrpc_value *resp = NULL;
+    xmlrpc_env env;
+    xmlrpc_int32 retval = 0;
+    QInt *qint;
+
+    TRACE("called");
+
+    if (resp_data == NULL) {
+        LOG("error handling RPC request");
+        return;
+    }
+
+    xmlrpc_env_init(&env);
+    resp = xmlrpc_parse_response(&env, resp_data, resp_data_len);
+    if (va_rpc_has_error(&env)) {
+        LOG("error parsing RPC response");
+        return;
+    }
+
+    xmlrpc_parse_value(&env, resp, "i", &retval);
+    if (va_rpc_has_error(&env)) {
+        retval = -1;
+        goto out;
+    }
+
+out:
+    qint = qint_from_int(retval);
+    xmlrpc_DECREF(resp);
+    if (mon_cb) {
+        mon_cb(mon_data, QOBJECT(qint));
+    }
+    qobject_decref(QOBJECT(qint));
+}
+
+int do_agent_fsstatus(Monitor *mon, const QDict *mon_params,
+                      MonitorCompletion cb, void *opaque)
+{
+    xmlrpc_env env;
+    xmlrpc_value *params;
+    int ret;
+
+    TRACE("called");
+
+    xmlrpc_env_init(&env);
+    params = xmlrpc_build_value(&env, "()");
+    if (va_rpc_has_error(&env)) {
+        return -1;
+    }
+
+    ret = va_do_rpc(&env, "va.fsstatus", params, do_agent_fsstatus_cb,
+                    cb, opaque);
+    if (ret) {
+        qerror_report(QERR_VA_FAILED, ret, strerror(ret));
+    }
+    xmlrpc_DECREF(params);
+    return ret;
+}
diff --git a/virtagent.h b/virtagent.h
index dba90d0..0d7575d 100644
--- a/virtagent.h
+++ b/virtagent.h
@@ -46,5 +46,14 @@ int do_agent_capabilities(Monitor *mon, const QDict *mon_params,
                   MonitorCompletion cb, void *opaque);
 int va_client_init_capabilities(void);
 int va_send_hello(void);
+void do_agent_fsfreeze_print(Monitor *mon, const QObject *qobject);
+int do_agent_fsfreeze(Monitor *mon, const QDict *mon_params,
+                      MonitorCompletion cb, void *opaque);
+void do_agent_fsthaw_print(Monitor *mon, const QObject *qobject);
+int do_agent_fsthaw(Monitor *mon, const QDict *mon_params,
+                    MonitorCompletion cb, void *opaque);
+void do_agent_fsstatus_print(Monitor *mon, const QObject *qobject);
+int do_agent_fsstatus(Monitor *mon, const QDict *mon_params,
+                      MonitorCompletion cb, void *opaque);
 
 #endif /* VIRTAGENT_H */
-- 
1.7.3.5

[Qemu-devel] Re: [PATCH 1/2] Add virtagent file system freeze/thaw

[Michael Roth]

On 02/02/2011 02:42 AM, Jes.Sorensen@redhat.com wrote:
> From: Jes Sorensen<Jes.Sorensen@redhat.com>
>
> Implement freeze/thaw support in the guest, allowing the host to
> request the guest freezes all it's file systems before a live snapshot
> is performed.
>   - fsfreeze(): Walk the list of mounted local real file systems,
>                 and freeze them.
>   - fsthaw():   Walk the list of previously frozen file systems and
>                 thaw them.
>   - fsstatus(): Return the current status of freeze/thaw. The host must
>                 poll this function, in case fsfreeze() returned with a
> 	       timeout, to wait for the operation to finish.
>
> Signed-off-by: Jes Sorensen<Jes.Sorensen@redhat.com>
> ---
>   virtagent-common.h |    8 ++
>   virtagent-server.c |  190 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 198 insertions(+), 0 deletions(-)
>
> diff --git a/virtagent-common.h b/virtagent-common.h
> index 5d8f5c1..7c6d9ef 100644
> --- a/virtagent-common.h
> +++ b/virtagent-common.h
> @@ -61,6 +61,14 @@ typedef struct VAContext {
>       const char *channel_path;
>   } VAContext;
>
> +enum va_fsfreeze_status {
> +    FREEZE_ERROR = -1,
> +    FREEZE_THAWED = 0,
> +    FREEZE_INPROGRESS = 1,
> +    FREEZE_FROZEN = 2,
> +    FREEZE_THAWINPROGRESS = 3,
> +};
> +
>   enum va_job_status {
>       VA_JOB_STATUS_PENDING = 0,
>       VA_JOB_STATUS_OK,
> diff --git a/virtagent-server.c b/virtagent-server.c
> index 7bb35b2..ffbe163 100644
> --- a/virtagent-server.c
> +++ b/virtagent-server.c
> @@ -14,6 +14,10 @@
>   #include<syslog.h>
>   #include "qemu_socket.h"
>   #include "virtagent-common.h"
> +#include<mntent.h>
> +#include<sys/types.h>
> +#include<sys/ioctl.h>
> +#include<linux/fs.h>
>
>   static VAServerData *va_server_data;
>   static bool va_enable_syslog = false; /* enable syslog'ing of RPCs */
> @@ -217,6 +221,186 @@ static xmlrpc_value *va_hello(xmlrpc_env *env,
>       return result;
>   }
>
> +
> +/*
> + * Walk the mount table and build a list of local file systems
> + */
> +
> +struct direntry {
> +    char *dirname;
> +    char *devtype;
> +    struct direntry *next;
> +};
> +
> +static struct direntry *va_mount_list;
> +static int va_fsfreeze_status;

And what I meant in the last RFC about using "objects" was to 
encapsulate global state information for a particular group of commands 
in single data type/variable. We're gonna end up with a similar set of 
variables for stateful RPCs like copyfile and potentially a few for 
things like spice. So to avoid having things get too cluttered up I'd 
prefer something like, in this particular case:

typedef struct VAFSFreezeState {
     struct direntry *mount_list;
     int status;
} VAFSFeezeState;

static VAFSFreezeState va_fsfreeze_state;

> +
> +static int build_mount_list(void)
> +{
> +    struct mntent *mnt;
> +    struct direntry *entry;
> +    struct direntry *next;
> +    char const *mtab = MOUNTED;
> +    FILE *fp;
> +
> +    fp = setmntent(mtab, "r");
> +    if (!fp) {
> +	fprintf(stderr, "unable to read mtab\n");
> +	goto fail;
> +    }

You have tabs instead of spaces here

> +
> +    while ((mnt = getmntent(fp))) {
> +	/*
> +	 * An entry which device name doesn't start with a '/' is
> +	 * either a dummy file system or a network file system.
> +	 * Add special handling for smbfs and cifs as is done by
> +	 * coreutils as well.
> +	 */
> +	if ((mnt->mnt_fsname[0] != '/') ||
> +	    (strcmp(mnt->mnt_type, "smbfs") == 0) ||
> +	    (strcmp(mnt->mnt_type, "cifs") == 0)) {
> +	    continue;
> +	}
> +
> +	entry = qemu_malloc(sizeof(struct direntry));
> +	entry->dirname = qemu_strdup(mnt->mnt_dir);
> +	entry->devtype = qemu_strdup(mnt->mnt_type);
> +	entry->next = va_mount_list;
> +
> +	va_mount_list = entry;

Here too

> +    }
> +
> +    endmntent(fp);
> +
> +    return 0;
> +
> +fail:
> +    while(va_mount_list) {
> +	next = va_mount_list->next;

Here too. Might be some other spots but you get the point :)

> +        qemu_free(va_mount_list->dirname);
> +        qemu_free(va_mount_list->devtype);
> +        qemu_free(va_mount_list);
> +        va_mount_list = next;
> +    }
> +
> +    return -1;
> +}
> +
> +/*
> + * va_fsfreeze(): Walk list of mounted file systems in the guest, and
> + *   freeze the ones which are real local file systems.
> + * rpc return values: Number of file systems frozen, -1 on error.
> + */
> +static xmlrpc_value *va_fsfreeze(xmlrpc_env *env,
> +                                 xmlrpc_value *params,
> +                                 void *user_data)
> +{
> +    xmlrpc_int32 ret = 0, i = 0;
> +    xmlrpc_value *result;
> +    struct direntry *entry;
> +    int fd;
> +    SLOG("va_fsfreeze()");
> +
> +    if (va_fsfreeze_status != FREEZE_THAWED) {
> +        ret = 0;
> +        goto out;
> +    }
> +
> +    ret = build_mount_list();
> +    if (ret<  0) {
> +        goto out;
> +    }
> +
> +    va_fsfreeze_status = FREEZE_INPROGRESS;
> +
> +    entry = va_mount_list;
> +    while(entry) {
> +        fd = qemu_open(entry->dirname, O_RDONLY);
> +        if (fd == -1) {
> +            ret = errno;
> +            goto error;
> +        }
> +        ret = ioctl(fd, FIFREEZE);
> +        close(fd);
> +        if (ret<  0&&  ret != EOPNOTSUPP) {
> +            goto error;
> +        }
> +
> +        entry = entry->next;
> +        i++;
> +    }
> +
> +    va_fsfreeze_status = FREEZE_FROZEN;
> +    ret = i;
> +out:
> +    result = xmlrpc_build_value(env, "i", ret);
> +    return result;
> +error:
> +    if (i>  0) {
> +        va_fsfreeze_status = FREEZE_ERROR;
> +    }
> +    goto out;
> +}
> +
> +/*
> + * va_fsthaw(): Walk list of frozen file systems in the guest, and
> + *   thaw them.
> + * rpc return values: Number of file systems thawed on success, -1 on error.
> + */
> +static xmlrpc_value *va_fsthaw(xmlrpc_env *env,
> +                               xmlrpc_value *params,
> +                               void *user_data)
> +{
> +    xmlrpc_int32 ret;
> +    xmlrpc_value *result;
> +    struct direntry *entry;
> +    int fd, i = 0;
> +    SLOG("va_fsthaw()");
> +
> +    if (va_fsfreeze_status != FREEZE_FROZEN) {
> +        ret = 0;
> +        goto out;
> +    }
> +
> +    while((entry = va_mount_list)) {
> +        fd = qemu_open(entry->dirname, O_RDONLY);
> +        if (fd == -1) {
> +            ret = -1;
> +            goto out;
> +        }
> +        ret = ioctl(fd, FITHAW);
> +        close(fd);
> +        if (ret<  0&&  ret != EOPNOTSUPP) {
> +            ret = -1;
> +            goto out;
> +        }
> +
> +        va_mount_list = entry->next;
> +        qemu_free(entry->dirname);
> +        qemu_free(entry->devtype);
> +        qemu_free(entry);
> +        i++;
> +    }
> +
> +    va_fsfreeze_status = FREEZE_THAWED;
> +    ret = i;
> +out:
> +    result = xmlrpc_build_value(env, "i", ret);
> +    return result;
> +}
> +
> +/* va_fsstatus(): Return status of freeze/thaw
> + * rpc return values: fsfreeze_status
> + */
> +static xmlrpc_value *va_fsstatus(xmlrpc_env *env,
> +                                 xmlrpc_value *params,
> +                                 void *user_data)
> +{
> +    xmlrpc_value *result = xmlrpc_build_value(env, "i", va_fsfreeze_status);
> +    SLOG("va_fsstatus()");
> +    return result;
> +}
> +
>   typedef struct RPCFunction {
>       xmlrpc_value *(*func)(xmlrpc_env *env, xmlrpc_value *param, void *unused);
>       const char *func_name;
> @@ -237,6 +421,12 @@ static RPCFunction guest_functions[] = {
>         .func_name = "va.ping" },
>       { .func = va_capabilities,
>         .func_name = "va.capabilities" },
> +    { .func = va_fsfreeze,
> +      .func_name = "va.fsfreeze" },
> +    { .func = va_fsthaw,
> +      .func_name = "va.fsthaw" },
> +    { .func = va_fsstatus,
> +      .func_name = "va.fsstatus" },
>       { NULL, NULL }
>   };
>   static RPCFunction host_functions[] = {

[Qemu-devel] Re: [PATCH v2 0/2] virtagent - fsfreeze support

[Michael Roth]

Hi Jes,

Can you add Badari (CC'd) to the next round? He's looked at potentially 
using virtagent for snapshots in the past and might have some insights 
here. Thanks.

On 02/02/2011 02:42 AM, Jes.Sorensen@redhat.com wrote:
> From: Jes Sorensen<Jes.Sorensen@redhat.com>
>
> Hi
>
> This is a first attempt to add fsfreeze support to virtagent. The idea
> is for the guest agent to walk the list of locally mounted file
> systems in the guest, and issuing an ioctl to freeze them. The host
> can then do a live snapshot of the guest, obtaining stable file
> systems. After the snapshot, the host then calls the thaw function in
> virtagent, which goes through the list of previously frozen file
> systems and unfreezes them.
>
> The list walking ignores remote file systems such as NFS and CIFS as
> well as all pseudo file systems.
>
> The guest agent code is in the first patch, and host agent code is in
> the second patch. For now there is only human monitor support, but it
> should be pretty straight forward to add QMP support as well.
>
> Comments and suggestions welcome!
>
> v2 of the patch addresses the issues pointed out by Stefan and Michael.
>
> Cheers,
> Jes
>
>
> Jes Sorensen (2):
>    Add virtagent file system freeze/thaw
>    Add monitor commands for fsfreeze support
>
>   hmp-commands.hx    |   48 +++++++++++
>   virtagent-common.h |    9 ++
>   virtagent-server.c |  190 ++++++++++++++++++++++++++++++++++++++++++
>   virtagent.c        |  235 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>   virtagent.h        |    9 ++
>   5 files changed, 491 insertions(+), 0 deletions(-)
>

[Qemu-devel] Re: [PATCH 1/2] Add virtagent file system freeze/thaw

[Jes Sorensen]

On 02/03/11 19:11, Michael Roth wrote:
>> @@ -217,6 +221,186 @@ static xmlrpc_value *va_hello(xmlrpc_env *env,
>>       return result;
>>   }
>>
>> +
>> +/*
>> + * Walk the mount table and build a list of local file systems
>> + */
>> +
>> +struct direntry {
>> +    char *dirname;
>> +    char *devtype;
>> +    struct direntry *next;
>> +};
>> +
>> +static struct direntry *va_mount_list;
>> +static int va_fsfreeze_status;
> 
> And what I meant in the last RFC about using "objects" was to
> encapsulate global state information for a particular group of commands
> in single data type/variable. We're gonna end up with a similar set of
> variables for stateful RPCs like copyfile and potentially a few for
> things like spice. So to avoid having things get too cluttered up I'd
> prefer something like, in this particular case:
> 
> typedef struct VAFSFreezeState {
>     struct direntry *mount_list;
>     int status;
> } VAFSFeezeState;
> 
> static VAFSFreezeState va_fsfreeze_state;

Ok, I got rid of the tabs (damn I thought I had caught them all), and
added a struct to keep the freeze state. I didn't add any typedef
grossness though.

v3 coming up.

Cheers,
Jes

[Qemu-devel] Re: [PATCH 1/2] Add virtagent file system freeze/thaw

[Jes Sorensen]

On 02/01/11 17:50, Michael Roth wrote:
> On 02/01/2011 04:58 AM, Jes.Sorensen@redhat.com wrote:
>> +enum vs_fsfreeze_status {
>> +    FREEZE_ERROR = -1,
>> +    FREEZE_THAWED = 0,
>> +    FREEZE_INPROGRESS = 1,
>> +    FREEZE_FROZEN = 2,
>> +    FREEZE_THAWINPROGRESS = 3,
>> +};
> 
> Any reason for vs_* vs. va_*?

Hmmmm let me see if I can find a good excuse for that typo :)

>> diff --git a/virtagent-server.c b/virtagent-server.c
>> index 7bb35b2..cf2a3f0 100644
>> --- a/virtagent-server.c
>> +++ b/virtagent-server.c
>> @@ -14,6 +14,13 @@
>>   #include<syslog.h>
>>   #include "qemu_socket.h"
>>   #include "virtagent-common.h"
>> +#include<mntent.h>
>> +#include<sys/types.h>
>> +#include<sys/stat.h>
>> +#include<sys/errno.h>
>> +#include<sys/ioctl.h>
>> +#include<fcntl.h>
>> +#include<linux/fs.h>
> 
> Can probably clean these up a bit, I believe fcntl.h/errno.h/stat.h are
> already available at least.

Carry-over from writing the code outside of qemu. Would be much cleaner
than relying on the include everything and the kitchen sink in a global
header file, but thats how it is :(

>> +
>> +    fsfreeze_status = FREEZE_INPROGRESS;
>> +
>> +    entry = mount_list;
> 
> I think as we start adding more and more stateful RPCs, free-floating
> state variables can start getting a bit hairy to keep track of.
> Eventually I'd like to have state information that only applies to a
> subset of RPCs consolidated into a single object. I wouldn't focus on
> this too much because I'd like to have an interface to do this in the
> future (mainly so they can state objects can register themselves and
> provide a reset() function that can be called when, for instance, an
> agent disconnects/reconnects), but in the meantime I think it would be
> more readable to have a global va_fsfreeze_state object to track freeze
> status and mount points.

Urgh, what do you mean by object here? I have to admit the word object
always makes me cringe.... I changed the variables to have the va_ prefix.

>> +static xmlrpc_value *va_fsstatus(xmlrpc_env *env,
>> +                                 xmlrpc_value *params,
>> +                                 void *user_data)
>> +{
>> +    xmlrpc_value *result = xmlrpc_build_value(env, "i",
>> fsfreeze_status);
>> +    SLOG("va_fsstatus()");
>> +    return result;
>> +}
> 
> Hmm, you mentioned before that these freezes may be long-running
> jobs...do the ioctl()'s not return until completion? There is global
> timeout in virtagent, currently under a minute, to prevent a virtagent
> monitor command from hanging the monitor session, so if it's unlikely
> you'll fit in this window we'll need to work on something to better
> support these this kinds of situations.

I think 1 minute is fine, but we should probably look at something a
little more flexible for handling commands over the longer term. Maybe
have virtagent spawn threads for executing some commands?

> The 3 main approaches would be:
> 
> 1) allow command-specific timeouts with values that are sane for the
> command in question, and potentially allow timeouts to be disabled
> 2) fork() long running jobs and provide a mechanism for them to provide
> asynchronous updates to us to we can query status
> 3) fork() long running jobs, have them provide status information
> elsewhere, and provide a polling function to check that status
> 
> 3) would likely require something like writing status to a file and then
> provide a polling function to check it, which doesn't work here so
> that's probably out.
> 
> I'd initially planned on doing 2) at some point, but I'm beginning to
> think 1) is the better approach, since qemu "opts in" on how long it's
> willing to hang for a particular command, so there's not really any
> surprises. At least not to qemu...users might get worried after a while,
> so there is a bit of a trade-off. But it's also more user-friendly....no
> need for polling or dealing with asynchronous updates to figure out when
> an RPC has actually finished. Seem reasonable?

I am not sure which is really the best solution. Basically we will need
to classify commands into two categories, so if you issue a certain type
of command, like agent_fsfreeze() (basically when the agent is in
FREEZE_FROZEN state) only status commands are allowed to execute in
parallel. Anything that tries to issue a write to the file system will
hang until agent_fsthaw is called. However it would be useful to be able
to call in for non-blocking status commands etc.

I'll post a v2 in a minute that addresses the issues pointed out by
Stefan and you. I think the threading/timeout aspect is something we
need to look at for the longer term.

Cheers,
Jes

[Qemu-devel] Re: [PATCH 1/2] Add virtagent file system freeze/thaw

[Michael Roth]

On 02/01/2011 04:58 AM, Jes.Sorensen@redhat.com wrote:
> From: Jes Sorensen<Jes.Sorensen@redhat.com>
>
> Implement freeze/thaw support in the guest, allowing the host to
> request the guest freezes all it's file systems before a live snapshot
> is performed.
>   - fsfreeze(): Walk the list of mounted local real file systems,
>                 and freeze them.
>   - fsthaw():   Walk the list of previously frozen file systems and
>                 thaw them.
>   - fsstatus(): Return the current status of freeze/thaw. The host must
>                 poll this function, in case fsfreeze() returned with a
> 	       timeout, to wait for the operation to finish.
>
> Signed-off-by: Jes Sorensen<Jes.Sorensen@redhat.com>
> ---
>   virtagent-common.h |    8 ++
>   virtagent-server.c |  196 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 204 insertions(+), 0 deletions(-)
>
> diff --git a/virtagent-common.h b/virtagent-common.h
> index 5d8f5c1..220a4b6 100644
> --- a/virtagent-common.h
> +++ b/virtagent-common.h
> @@ -61,6 +61,14 @@ typedef struct VAContext {
>       const char *channel_path;
>   } VAContext;
>
> +enum vs_fsfreeze_status {
> +    FREEZE_ERROR = -1,
> +    FREEZE_THAWED = 0,
> +    FREEZE_INPROGRESS = 1,
> +    FREEZE_FROZEN = 2,
> +    FREEZE_THAWINPROGRESS = 3,
> +};

Any reason for vs_* vs. va_*?

> +
>   enum va_job_status {
>       VA_JOB_STATUS_PENDING = 0,
>       VA_JOB_STATUS_OK,
> diff --git a/virtagent-server.c b/virtagent-server.c
> index 7bb35b2..cf2a3f0 100644
> --- a/virtagent-server.c
> +++ b/virtagent-server.c
> @@ -14,6 +14,13 @@
>   #include<syslog.h>
>   #include "qemu_socket.h"
>   #include "virtagent-common.h"
> +#include<mntent.h>
> +#include<sys/types.h>
> +#include<sys/stat.h>
> +#include<sys/errno.h>
> +#include<sys/ioctl.h>
> +#include<fcntl.h>
> +#include<linux/fs.h>

Can probably clean these up a bit, I believe fcntl.h/errno.h/stat.h are 
already available at least.

>
>   static VAServerData *va_server_data;
>   static bool va_enable_syslog = false; /* enable syslog'ing of RPCs */
> @@ -217,6 +224,189 @@ static xmlrpc_value *va_hello(xmlrpc_env *env,
>       return result;
>   }
>
> +
> +/*
> + * Walk the mount table and build a list of local file systems
> + */
> +
> +struct direntry {
> +    char *dirname;
> +    char *devtype;
> +    struct direntry *next;
> +};
> +
> +static struct direntry *mount_list;
> +static int fsfreeze_status;
> +
> +static int build_mount_list(void)
> +{
> +    struct mntent *mnt;
> +    struct direntry *entry;
> +    struct direntry *next;
> +    char const *mtab = MOUNTED;
> +    FILE *fp;
> +
> +    fp = setmntent(mtab, "r");
> +    if (!fp) {
> +	fprintf(stderr, "unable to read mtab\n");
> +	goto fail;
> +    }
> +
> +    while ((mnt = getmntent(fp))) {
> +	/*
> +	 * An entry which device name doesn't start with a '/' is
> +	 * either a dummy file system or a network file system.
> +	 * Add special handling for smbfs and cifs as is done by
> +	 * coreutils as well.
> +	 */
> +	if ((mnt->mnt_fsname[0] != '/') ||
> +	    (strcmp(mnt->mnt_type, "smbfs") == 0) ||
> +	    (strcmp(mnt->mnt_type, "cifs") == 0)) {
> +	    continue;
> +	}
> +
> +	entry = qemu_malloc(sizeof(struct direntry));
> +	if (!entry) {
> +	    goto fail;
> +	}
> +	entry->dirname = qemu_strdup(mnt->mnt_dir);
> +	entry->devtype = qemu_strdup(mnt->mnt_type);
> +	entry->next = mount_list;
> +
> +	mount_list = entry;
> +    }
> +
> +    endmntent(fp);
> +
> +    return 0;
> +
> +fail:
> +    while(mount_list) {
> +	next = mount_list->next;
> +	qemu_free(mount_list->dirname);
> +	qemu_free(mount_list->devtype);
> +	qemu_free(mount_list);
> +	mount_list = next;
> +    }

should be spaces instead of tabs

> +
> +    return -1;
> +}
> +
> +/*
> + * va_fsfreeze(): Walk list of mounted file systems in the guest, and
> + *   freeze the ones which are real local file systems.
> + * rpc return values: Number of file systems frozen, -1 on error.
> + */
> +static xmlrpc_value *va_fsfreeze(xmlrpc_env *env,
> +                                 xmlrpc_value *params,
> +                                 void *user_data)
> +{
> +    xmlrpc_int32 ret = 0, i = 0;
> +    xmlrpc_value *result;
> +    struct direntry *entry;
> +    int fd;
> +    SLOG("va_fsfreeze()");
> +
> +    if (fsfreeze_status == FREEZE_FROZEN) {
> +        ret = 0;
> +        goto out;
> +    }
> +
> +    ret = build_mount_list();
> +    if (ret<  0) {
> +        goto out;
> +    }
> +
> +    fsfreeze_status = FREEZE_INPROGRESS;
> +
> +    entry = mount_list;

I think as we start adding more and more stateful RPCs, free-floating 
state variables can start getting a bit hairy to keep track of. 
Eventually I'd like to have state information that only applies to a 
subset of RPCs consolidated into a single object. I wouldn't focus on 
this too much because I'd like to have an interface to do this in the 
future (mainly so they can state objects can register themselves and 
provide a reset() function that can be called when, for instance, an 
agent disconnects/reconnects), but in the meantime I think it would be 
more readable to have a global va_fsfreeze_state object to track freeze 
status and mount points.

> +    while(entry) {
> +        fd = qemu_open(entry->dirname, O_RDONLY);
> +        if (fd == -1) {
> +            ret = errno;
> +            goto error;
> +        }
> +        ret = ioctl(fd, FIFREEZE);
> +        if (ret<  0&&  ret != EOPNOTSUPP) {
> +            goto error;
> +        }
> +
> +        close(fd);
> +        entry = entry->next;
> +        i++;
> +    }
> +
> +    fsfreeze_status = FREEZE_FROZEN;
> +    ret = i;
> +out:
> +    result = xmlrpc_build_value(env, "i", ret);
> +    return result;
> +error:
> +    if (i>  0) {
> +        fsfreeze_status = FREEZE_ERROR;
> +    }
> +    goto out;
> +}
> +
> +/*
> + * va_fsthaw(): Walk list of frozen file systems in the guest, and
> + *   thaw them.
> + * rpc return values: Number of file systems thawed on success, -1 on error.
> + */
> +static xmlrpc_value *va_fsthaw(xmlrpc_env *env,
> +                               xmlrpc_value *params,
> +                               void *user_data)
> +{
> +    xmlrpc_int32 ret;
> +    xmlrpc_value *result;
> +    struct direntry *entry;
> +    int fd, i = 0;
> +    SLOG("va_fsthaw()");
> +
> +    if (fsfreeze_status == FREEZE_THAWED) {
> +        ret = 0;
> +        goto out;
> +    }
> +
> +    while((entry = mount_list)) {
> +        fd = qemu_open(entry->dirname, O_RDONLY);
> +        if (fd == -1) {
> +            ret = -1;
> +            goto out;
> +        }
> +        ret = ioctl(fd, FITHAW);
> +        if (ret<  0&&  ret != EOPNOTSUPP) {
> +            ret = -1;
> +            goto out;
> +	}

whitespace issues

> +        close(fd);
> +
> +        mount_list = entry->next;
> +        qemu_free(entry->dirname);
> +        qemu_free(entry->devtype);
> +        qemu_free(entry);
> +        i++;
> +    }
> +
> +    fsfreeze_status = FREEZE_THAWED;
> +    ret = i;
> +out:
> +    result = xmlrpc_build_value(env, "i", ret);
> +    return result;
> +}
> +
> +/* va_fsstatus(): Return status of freeze/thaw
> + * rpc return values: fsfreeze_status
> + */
> +static xmlrpc_value *va_fsstatus(xmlrpc_env *env,
> +                                 xmlrpc_value *params,
> +                                 void *user_data)
> +{
> +    xmlrpc_value *result = xmlrpc_build_value(env, "i", fsfreeze_status);
> +    SLOG("va_fsstatus()");
> +    return result;
> +}

Hmm, you mentioned before that these freezes may be long-running 
jobs...do the ioctl()'s not return until completion? There is global 
timeout in virtagent, currently under a minute, to prevent a virtagent 
monitor command from hanging the monitor session, so if it's unlikely 
you'll fit in this window we'll need to work on something to better 
support these this kinds of situations.

The 3 main approaches would be:

1) allow command-specific timeouts with values that are sane for the 
command in question, and potentially allow timeouts to be disabled
2) fork() long running jobs and provide a mechanism for them to provide 
asynchronous updates to us to we can query status
3) fork() long running jobs, have them provide status information 
elsewhere, and provide a polling function to check that status

3) would likely require something like writing status to a file and then 
provide a polling function to check it, which doesn't work here so 
that's probably out.

I'd initially planned on doing 2) at some point, but I'm beginning to 
think 1) is the better approach, since qemu "opts in" on how long it's 
willing to hang for a particular command, so there's not really any 
surprises. At least not to qemu...users might get worried after a while, 
so there is a bit of a trade-off. But it's also more user-friendly....no 
need for polling or dealing with asynchronous updates to figure out when 
an RPC has actually finished. Seem reasonable?

> +
>   typedef struct RPCFunction {
>       xmlrpc_value *(*func)(xmlrpc_env *env, xmlrpc_value *param, void *unused);
>       const char *func_name;
> @@ -237,6 +427,12 @@ static RPCFunction guest_functions[] = {
>         .func_name = "va.ping" },
>       { .func = va_capabilities,
>         .func_name = "va.capabilities" },
> +    { .func = va_fsfreeze,
> +      .func_name = "va.fsfreeze" },
> +    { .func = va_fsthaw,
> +      .func_name = "va.fsthaw" },
> +    { .func = va_fsstatus,
> +      .func_name = "va.fsstatus" },
>       { NULL, NULL }
>   };
>   static RPCFunction host_functions[] = {

[Qemu-devel] Re: [PATCH 1/2] Add virtagent file system freeze/thaw

[Jes Sorensen]

On 02/01/11 15:48, Adam Litke wrote:
> On Tue, 2011-02-01 at 11:58 +0100, Jes.Sorensen@redhat.com wrote:
>> +/*
>> + * va_fsfreeze(): Walk list of mounted file systems in the guest, and
>> + *   freeze the ones which are real local file systems.
>> + * rpc return values: Number of file systems frozen, -1 on error.
>> + */
>> +static xmlrpc_value *va_fsfreeze(xmlrpc_env *env,
>> +                                 xmlrpc_value *params,
>> +                                 void *user_data)
>> +{
>> +    xmlrpc_int32 ret = 0, i = 0;
>> +    xmlrpc_value *result;
>> +    struct direntry *entry;
>> +    int fd;
>> +    SLOG("va_fsfreeze()");
>> +
>> +    if (fsfreeze_status == FREEZE_FROZEN) {
>> +        ret = 0;
>> +        goto out;
>> +    }
>> +
>> +    ret = build_mount_list();
>> +    if (ret < 0) {
>> +        goto out;
>> +    }
>> +
>> +    fsfreeze_status = FREEZE_INPROGRESS;
>> +
>> +    entry = mount_list;
>> +    while(entry) {
>> +        fd = qemu_open(entry->dirname, O_RDONLY);
>> +        if (fd == -1) {
>> +            ret = errno;
>> +            goto error;
>> +        }
>> +        ret = ioctl(fd, FIFREEZE);
>> +        if (ret < 0 && ret != EOPNOTSUPP) {
>> +            goto error;
>> +        }
> 
> Here we silently ignore filesystems that do not support the FIFREEZE
> ioctl.  Do we need to have a more complex return value so that we can
> communicate which mount points could not be frozen?  Otherwise, an
> unsuspecting host could retrieve a corrupted snapshot of that
> filesystem, right?

That is correct, however most Linux file systems do support it, and for
the ones that don't, there really isn't anything we can do.

Cheers,
Jes

[Qemu-devel] Re: [PATCH 1/2] Add virtagent file system freeze/thaw

[Adam Litke]

On Tue, 2011-02-01 at 11:58 +0100, Jes.Sorensen@redhat.com wrote:
> +/*
> + * va_fsfreeze(): Walk list of mounted file systems in the guest, and
> + *   freeze the ones which are real local file systems.
> + * rpc return values: Number of file systems frozen, -1 on error.
> + */
> +static xmlrpc_value *va_fsfreeze(xmlrpc_env *env,
> +                                 xmlrpc_value *params,
> +                                 void *user_data)
> +{
> +    xmlrpc_int32 ret = 0, i = 0;
> +    xmlrpc_value *result;
> +    struct direntry *entry;
> +    int fd;
> +    SLOG("va_fsfreeze()");
> +
> +    if (fsfreeze_status == FREEZE_FROZEN) {
> +        ret = 0;
> +        goto out;
> +    }
> +
> +    ret = build_mount_list();
> +    if (ret < 0) {
> +        goto out;
> +    }
> +
> +    fsfreeze_status = FREEZE_INPROGRESS;
> +
> +    entry = mount_list;
> +    while(entry) {
> +        fd = qemu_open(entry->dirname, O_RDONLY);
> +        if (fd == -1) {
> +            ret = errno;
> +            goto error;
> +        }
> +        ret = ioctl(fd, FIFREEZE);
> +        if (ret < 0 && ret != EOPNOTSUPP) {
> +            goto error;
> +        }

Here we silently ignore filesystems that do not support the FIFREEZE
ioctl.  Do we need to have a more complex return value so that we can
communicate which mount points could not be frozen?  Otherwise, an
unsuspecting host could retrieve a corrupted snapshot of that
filesystem, right?

> +
> +        close(fd);
> +        entry = entry->next;
> +        i++;
> +    }
> +
> +    fsfreeze_status = FREEZE_FROZEN;
> +    ret = i;
> +out:
> +    result = xmlrpc_build_value(env, "i", ret);
> +    return result;
> +error:
> +    if (i > 0) {
> +        fsfreeze_status = FREEZE_ERROR;
> +    }
> +    goto out;
> +}

-- 
Thanks,
Adam