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

2011-02-01 Thread Adam Litke 
On Tue, 2011-02-01 at 11:58 +0100, jes.soren...@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

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

2011-02-01 Thread Jes Sorensen 
On 02/01/11 15:48, Adam Litke wrote:
> On Tue, 2011-02-01 at 11:58 +0100, jes.soren...@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

2011-02-01 Thread Michael Roth 

On 02/01/2011 04:58 AM, jes.soren...@redhat.com wrote:

From: Jes Sorensen

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
---
  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
  #include "qemu_socket.h"
  #include "virtagent-common.h"
+#include
+#include
+#include
+#include
+#include
+#include
+#include


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_RDONL

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

2011-02-02 Thread Jes Sorensen 
On 02/01/11 17:50, Michael Roth wrote:
> On 02/01/2011 04:58 AM, jes.soren...@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_*?

H 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
>>   #include "qemu_socket.h"
>>   #include "virtagent-common.h"
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
> 
> 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-friendlyno
> 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

2011-02-03 Thread Michael Roth 

On 02/02/2011 02:42 AM, jes.soren...@redhat.com wrote:

From: Jes Sorensen

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
---
  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
  #include "qemu_socket.h"
  #include "virtagent-common.h"
+#include
+#include
+#include
+#include

  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;
+

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

2011-02-04 Thread 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