[ovs-dev] [PATCH 2/2] ovsdb-tool: Do not lock source db for compacting or converting to new db.
Justin Pettit
jpettit at nicira.com
Mon Mar 8 22:48:20 UTC 2010
Looks good. I may be a little slow today, but this line in the function was a bit confusing to me on a quick glance:
bool in_place = dst_name == NULL;
Giving it consideration, it's pretty obvious what's going on. Quickly reviewing the code, though, I read it as assigning NULL to in_place and dst_name. I just think it might be a bit clearer broken into two lines or even just adding a pair of parentheses. Feel free to ignore this suggestion if you think it's clear enough, though.
--Justin
On Mar 8, 2010, at 2:02 PM, Ben Pfaff wrote:
> "ovsdb-tool compact SRC DST" and "ovsdb-tool convert SRC SCHEMA DST" do not
> need to lock SRC, because they do not modify it.
>
> Reported-by: Justin Pettit <jpettit at nicira.com>
> ---
> ovsdb/ovsdb-tool.c | 16 ++++++++--------
> 1 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/ovsdb/ovsdb-tool.c b/ovsdb/ovsdb-tool.c
> index 26f9003..b2e200c 100644
> --- a/ovsdb/ovsdb-tool.c
> +++ b/ovsdb/ovsdb-tool.c
> @@ -187,17 +187,18 @@ compact_or_convert(const char *src_name, const char *dst_name,
> struct ovsdb *db;
> int retval;
>
> - /* Get (temporary) destination. */
> + /* Lock the source, if we will be replacing it. */
> if (in_place) {
> - dst_name = xasprintf("%s.tmp", src_name);
> + retval = lockfile_lock(src_name, INT_MAX, &src_lock);
> + if (retval) {
> + ovs_fatal(retval, "%s: failed to lock lockfile", src_name);
> + }
> }
>
> - /* Lock source and (temporary) destination. */
> - retval = lockfile_lock(src_name, INT_MAX, &src_lock);
> - if (retval) {
> - ovs_fatal(retval, "%s: failed to lock lockfile", src_name);
> + /* Get (temporary) destination and lock it. */
> + if (in_place) {
> + dst_name = xasprintf("%s.tmp", src_name);
> }
> -
> retval = lockfile_lock(dst_name, INT_MAX, &dst_lock);
> if (retval) {
> ovs_fatal(retval, "%s: failed to lock lockfile", dst_name);
> @@ -217,7 +218,6 @@ compact_or_convert(const char *src_name, const char *dst_name,
> dst_name, src_name);
> }
> fsync_parent_dir(dst_name);
> - } else {
> lockfile_unlock(src_lock);
> }
>
> --
> 1.6.6.1
>
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev_openvswitch.org
More information about the dev
mailing list