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_*?
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