Skip to content

L
lxc
Commit 39dc698c by Serge Hallyn
Options

lxccontainer: don't lock around getstate and freeze/unfreeze (v2)

Those go through commands.c and are already mutex'ed that way. Also remove a unmatched container_disk_unlock in lxcapi_create. Since is_stopped uses getstate which is no longer locked, rename it to drop the _locked suffix. And convert save_config to taking the disk lock. This way the save_ and load_config are mutexing each other, as they should. Changelog: May 29: Per Dwight's comment, take the lock before opening the config FILE *. Only take disklock at load and save_config when we're using the container's config file, not when read/writing from/to another file. Signed-off-by: 's avatarSerge Hallyn <serge.hallyn@ubuntu.com> Acked-by: 's avatarDwight Engen <dwight.engen@oracle.com>
parent 0115f8fd
Showing with 61 additions and 31 deletions
src/lxc/lxccontainer.c
...@@ -287,21 +287,15 @@ out: ...@@ -287,21 +287,15 @@ out:
static const char *lxcapi_state(struct lxc_container *c) static const char *lxcapi_state(struct lxc_container *c)
{ {
const char *ret;
lxc_state_t s; lxc_state_t s;
if (!c) if (!c)
return NULL; return NULL;
if (container_disk_lock(c))
return NULL;
s = lxc_getstate(c->name, c->config_path); s = lxc_getstate(c->name, c->config_path);
ret = lxc_state2str(s); return lxc_state2str(s);
container_disk_unlock(c);
return ret;
} }
static bool is_stopped_locked(struct lxc_container *c) static bool is_stopped(struct lxc_container *c)
{ {
lxc_state_t s; lxc_state_t s;
s = lxc_getstate(c->name, c->config_path); s = lxc_getstate(c->name, c->config_path);
...@@ -326,10 +320,7 @@ static bool lxcapi_freeze(struct lxc_container *c) ...@@ -326,10 +320,7 @@ static bool lxcapi_freeze(struct lxc_container *c)
if (!c) if (!c)
return false; return false;
if (container_disk_lock(c))
return false;
ret = lxc_freeze(c->name, c->config_path); ret = lxc_freeze(c->name, c->config_path);
container_disk_unlock(c);
if (ret) if (ret)
return false; return false;
return true; return true;
...@@ -341,10 +332,7 @@ static bool lxcapi_unfreeze(struct lxc_container *c) ...@@ -341,10 +332,7 @@ static bool lxcapi_unfreeze(struct lxc_container *c)
if (!c) if (!c)
return false; return false;
if (container_disk_lock(c))
return false;
ret = lxc_unfreeze(c->name, c->config_path); ret = lxc_unfreeze(c->name, c->config_path);
container_disk_unlock(c);
if (ret) if (ret)
return false; return false;
return true; return true;
...@@ -379,7 +367,8 @@ static bool load_config_locked(struct lxc_container *c, const char *fname) ...@@ -379,7 +367,8 @@ static bool load_config_locked(struct lxc_container *c, const char *fname)
static bool lxcapi_load_config(struct lxc_container *c, const char *alt_file) static bool lxcapi_load_config(struct lxc_container *c, const char *alt_file)
{ {
bool ret = false; bool ret = false, need_disklock = false;
int lret;
const char *fname; const char *fname;
if (!c) if (!c)
return false; return false;
...@@ -389,10 +378,27 @@ static bool lxcapi_load_config(struct lxc_container *c, const char *alt_file) ...@@ -389,10 +378,27 @@ static bool lxcapi_load_config(struct lxc_container *c, const char *alt_file)
fname = alt_file; fname = alt_file;
if (!fname) if (!fname)
return false; return false;
if (container_disk_lock(c)) /*
* If we're reading something other than the container's config,
* we only need to lock the in-memory container. If loading the
* container's config file, take the disk lock.
*/
if (strcmp(fname, c->configfile) == 0)
need_disklock = true;
if (need_disklock)
lret = container_disk_lock(c);
else
lret = container_mem_lock(c);
if (lret)
return false; return false;
ret = load_config_locked(c, fname); ret = load_config_locked(c, fname);
container_disk_unlock(c);
if (need_disklock)
container_disk_unlock(c);
else
container_mem_unlock(c);
return ret; return ret;
} }
...@@ -898,7 +904,6 @@ static bool lxcapi_create(struct lxc_container *c, const char *t, ...@@ -898,7 +904,6 @@ static bool lxcapi_create(struct lxc_container *c, const char *t,
out_unlock: out_unlock:
if (partial_fd >= 0) if (partial_fd >= 0)
remove_partial(c, partial_fd); remove_partial(c, partial_fd);
container_disk_unlock(c);
out: out:
if (tpath) if (tpath)
free(tpath); free(tpath);
...@@ -1153,30 +1158,55 @@ static int lxcapi_get_keys(struct lxc_container *c, const char *key, char *retv, ...@@ -1153,30 +1158,55 @@ static int lxcapi_get_keys(struct lxc_container *c, const char *key, char *retv,
#define LXC_DEFAULT_CONFIG "/etc/lxc/default.conf" #define LXC_DEFAULT_CONFIG "/etc/lxc/default.conf"
static bool lxcapi_save_config(struct lxc_container *c, const char *alt_file) static bool lxcapi_save_config(struct lxc_container *c, const char *alt_file)
{ {
FILE *fout;
bool ret = false, need_disklock = false;
int lret;
if (!alt_file) if (!alt_file)
alt_file = c->configfile; alt_file = c->configfile;
if (!alt_file) if (!alt_file)
return false; // should we write to stdout if no file is specified? return false; // should we write to stdout if no file is specified?
if (!c->lxc_conf)
// If we haven't yet loaded a config, load the stock config
if (!c->lxc_conf) {
if (!c->load_config(c, LXC_DEFAULT_CONFIG)) { if (!c->load_config(c, LXC_DEFAULT_CONFIG)) {
ERROR("Error loading default configuration file %s while saving %s\n", LXC_DEFAULT_CONFIG, c->name); ERROR("Error loading default configuration file %s while saving %s\n", LXC_DEFAULT_CONFIG, c->name);
return false; return false;
} }
}
if (!create_container_dir(c)) if (!create_container_dir(c))
return false; return false;
FILE *fout = fopen(alt_file, "w"); /*
if (!fout) * If we're writing to the container's config file, take the
return false; * disk lock. Otherwise just take the memlock to protect the
if (container_mem_lock(c)) { * struct lxc_container while we're traversing it.
fclose(fout); */
if (strcmp(c->configfile, alt_file) == 0)
need_disklock = true;
if (need_disklock)
lret = container_disk_lock(c);
else
lret = container_mem_lock(c);
if (lret)
return false; return false;
}
fout = fopen(alt_file, "w");
if (!fout)
goto out;
write_config(fout, c->lxc_conf); write_config(fout, c->lxc_conf);
fclose(fout); fclose(fout);
container_mem_unlock(c); ret = true;
return true;
out:
if (need_disklock)
container_disk_unlock(c);
else
container_mem_unlock(c);
return ret;
} }
// do we want the api to support --force, or leave that to the caller? // do we want the api to support --force, or leave that to the caller?
...@@ -1195,7 +1225,7 @@ static bool lxcapi_destroy(struct lxc_container *c) ...@@ -1195,7 +1225,7 @@ static bool lxcapi_destroy(struct lxc_container *c)
return false; return false;
} }
if (!is_stopped_locked(c)) { if (!is_stopped(c)) {
// we should queue some sort of error - in c->error_string? // we should queue some sort of error - in c->error_string?
ERROR("container %s is not stopped", c->name); ERROR("container %s is not stopped", c->name);
goto out; goto out;
...@@ -1352,7 +1382,7 @@ static bool lxcapi_set_cgroup_item(struct lxc_container *c, const char *subsys, ...@@ -1352,7 +1382,7 @@ static bool lxcapi_set_cgroup_item(struct lxc_container *c, const char *subsys,
if (container_mem_lock(c)) if (container_mem_lock(c))
return false; return false;
if (is_stopped_locked(c)) if (is_stopped(c))
goto err; goto err;
ret = lxc_cgroup_set(c->name, subsys, value, c->config_path); ret = lxc_cgroup_set(c->name, subsys, value, c->config_path);
...@@ -1373,7 +1403,7 @@ static int lxcapi_get_cgroup_item(struct lxc_container *c, const char *subsys, c ...@@ -1373,7 +1403,7 @@ static int lxcapi_get_cgroup_item(struct lxc_container *c, const char *subsys, c
if (container_mem_lock(c)) if (container_mem_lock(c))
return -1; return -1;
if (is_stopped_locked(c)) if (is_stopped(c))
goto out; goto out;
ret = lxc_cgroup_get(c->name, subsys, retv, inlen, c->config_path); ret = lxc_cgroup_get(c->name, subsys, retv, inlen, c->config_path);
...@@ -1837,7 +1867,7 @@ struct lxc_container *lxcapi_clone(struct lxc_container *c, const char *newname, ...@@ -1837,7 +1867,7 @@ struct lxc_container *lxcapi_clone(struct lxc_container *c, const char *newname,
if (container_mem_lock(c)) if (container_mem_lock(c))
return NULL; return NULL;
if (!is_stopped_locked(c)) { if (!is_stopped(c)) {
ERROR("error: Original container (%s) is running", c->name); ERROR("error: Original container (%s) is running", c->name);
goto out; goto out;
} }
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment