[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