[ovs-dev] [bug5144 5/5] ovsdb: Truncate bad transactions from database log.

Ethan Jackson ethan at nicira.com
Thu Mar 31 01:18:30 UTC 2011


Looks Good.

I didn't closely review the tests.

Ethan

On Mon, Mar 28, 2011 at 1:07 PM, Ben Pfaff <blp at nicira.com> wrote:
> When ovsdb-server reads a database file that is corrupted at the
> transaction level (that is, the transaction is valid JSON and has the
> correct SHA-1 hash, but it does not describe a valid database transaction),
> then ovsdb-server should truncate it and overwrite it by valid
> transactions.  However, until now, it didn't.  Instead, it would keep the
> invalid transaction and possibly every transaction in the database file
> (depending on in what way the transaction was invalid), which would just
> cause the same trouble again the next time the database was read.
>
> This fixes the problem.  An invalid transaction will be deleted from the
> database file at the first write to the database.
>
> Bug #5144.
> Bug #5149.
> ---
>  ovsdb/file.c          |    7 ++++++-
>  ovsdb/log.c           |   21 ++++++++++++++++++++-
>  ovsdb/log.h           |    4 +++-
>  tests/ovsdb-server.at |   48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 77 insertions(+), 3 deletions(-)
>
> diff --git a/ovsdb/file.c b/ovsdb/file.c
> index 1425beb..605e9cb 100644
> --- a/ovsdb/file.c
> +++ b/ovsdb/file.c
> @@ -215,6 +215,7 @@ ovsdb_file_open__(const char *file_name,
>                                          &date, &txn);
>         json_destroy(json);
>         if (error) {
> +            ovsdb_log_unread(log);
>             break;
>         }
>
> @@ -223,7 +224,11 @@ ovsdb_file_open__(const char *file_name,
>             oldest_commit = date;
>         }
>
> -        ovsdb_error_destroy(ovsdb_txn_commit(txn, false));
> +        error = ovsdb_txn_commit(txn, false);
> +        if (error) {
> +            ovsdb_log_unread(log);
> +            break;
> +        }
>     }
>     if (error) {
>         /* Log error but otherwise ignore it.  Probably the database just got
> diff --git a/ovsdb/log.c b/ovsdb/log.c
> index c0be5f5..6704307 100644
> --- a/ovsdb/log.c
> +++ b/ovsdb/log.c
> @@ -1,4 +1,4 @@
> -/* Copyright (c) 2009, 2010 Nicira Networks
> +/* Copyright (c) 2009, 2010, 2011 Nicira Networks
>  *
>  * Licensed under the Apache License, Version 2.0 (the "License");
>  * you may not use this file except in compliance with the License.
> @@ -43,6 +43,7 @@ enum ovsdb_log_mode {
>  };
>
>  struct ovsdb_log {
> +    off_t prev_offset;
>     off_t offset;
>     char *name;
>     struct lockfile *lockfile;
> @@ -121,6 +122,7 @@ ovsdb_log_open(const char *name, enum ovsdb_log_open_mode open_mode,
>     file->name = xstrdup(name);
>     file->lockfile = lockfile;
>     file->stream = stream;
> +    file->prev_offset = 0;
>     file->offset = 0;
>     file->read_error = NULL;
>     file->write_error = NULL;
> @@ -284,6 +286,7 @@ ovsdb_log_read(struct ovsdb_log *file, struct json **jsonp)
>         goto error;
>     }
>
> +    file->prev_offset = file->offset;
>     file->offset = data_offset + data_length;
>     *jsonp = json;
>     return 0;
> @@ -294,6 +297,22 @@ error:
>     return error;
>  }
>
> +/* Causes the log record read by the previous call to ovsdb_log_read() to be
> + * effectively discarded.  The next call to ovsdb_log_write() will overwrite
> + * that previously read record.
> + *
> + * Calling this function more than once has no additional effect.
> + *
> + * This function is useful when ovsdb_log_read() successfully reads a record
> + * but that record does not make sense at a higher level (e.g. it specifies an
> + * invalid transaction). */
> +void
> +ovsdb_log_unread(struct ovsdb_log *file)
> +{
> +    assert(file->mode == OVSDB_LOG_READ);
> +    file->offset = file->prev_offset;
> +}
> +
>  struct ovsdb_error *
>  ovsdb_log_write(struct ovsdb_log *file, struct json *json)
>  {
> diff --git a/ovsdb/log.h b/ovsdb/log.h
> index 0593fd8..f48dc76 100644
> --- a/ovsdb/log.h
> +++ b/ovsdb/log.h
> @@ -1,4 +1,4 @@
> -/* Copyright (c) 2009, 2010 Nicira Networks
> +/* Copyright (c) 2009, 2010, 2011 Nicira Networks
>  *
>  * Licensed under the Apache License, Version 2.0 (the "License");
>  * you may not use this file except in compliance with the License.
> @@ -36,6 +36,8 @@ void ovsdb_log_close(struct ovsdb_log *);
>
>  struct ovsdb_error *ovsdb_log_read(struct ovsdb_log *, struct json **)
>     WARN_UNUSED_RESULT;
> +void ovsdb_log_unread(struct ovsdb_log *);
> +
>  struct ovsdb_error *ovsdb_log_write(struct ovsdb_log *, struct json *)
>     WARN_UNUSED_RESULT;
>  struct ovsdb_error *ovsdb_log_commit(struct ovsdb_log *)
> diff --git a/tests/ovsdb-server.at b/tests/ovsdb-server.at
> index dff19ec..88499d0 100644
> --- a/tests/ovsdb-server.at
> +++ b/tests/ovsdb-server.at
> @@ -85,6 +85,54 @@ AT_CHECK([perl $srcdir/uuidfilt.pl output], [0],
>          [test ! -e pid || kill `cat pid`])
>  AT_CLEANUP
>
> +AT_SETUP([truncating database log with bad transaction])
> +AT_KEYWORDS([ovsdb server positive unix])
> +AT_DATA([schema], [ORDINAL_SCHEMA
> +])
> +AT_CHECK([ovsdb-tool create db schema], [0], [stdout], [ignore])
> +dnl Do one transaction and save the output.
> +AT_DATA([txnfile], [[ovsdb-client transact unix:socket \
> +'["ordinals",
> +  {"op": "insert",
> +   "table": "ordinals",
> +   "row": {"number": 0, "name": "zero"}}]'
> +]])
> +AT_CHECK([ovsdb-server -vlockfile:ANY:EMER --remote=punix:socket --unixctl=$PWD/unixctl db --run="sh txnfile"], [0], [stdout], [])
> +cat stdout >> output
> +dnl Add some crap to the database log and run another transaction, which should
> +dnl ignore the crap and truncate it out of the log.
> +echo 'OVSDB JSON 15 ffbcdae4b0386265f9ea3280dd7c8f0b72a20e56
> +{"invalid":{}}' >> db
> +AT_DATA([txnfile], [[ovsdb-client transact unix:socket \
> +'["ordinals",
> +  {"op": "insert",
> +   "table": "ordinals",
> +   "row": {"number": 1, "name": "one"}}]'
> +]])
> +AT_CHECK([ovsdb-server -vlockfile:ANY:EMER --remote=punix:socket --unixctl=$PWD/unixctl db --run="sh txnfile"], [0], [stdout], [stderr])
> +AT_CHECK([grep 'syntax "{"invalid":{}}": unknown table: No table named invalid.' stderr],
> +  [0], [ignore])
> +cat stdout >> output
> +dnl Run a final transaction to verify that both transactions succeeeded.
> +dnl The crap that we added should have been truncated by the previous run,
> +dnl so ovsdb-server shouldn't log a warning this time.
> +AT_DATA([txnfile], [[ovsdb-client transact unix:socket \
> +'["ordinals",
> +  {"op": "select",
> +   "table": "ordinals",
> +   "where": [],
> +   "sort": ["number"]}]'
> +]])
> +AT_CHECK([ovsdb-server -vlockfile:ANY:EMER --remote=punix:socket --unixctl=$PWD/unixctl db --run="sh txnfile"], [0], [stdout], [])
> +cat stdout >> output
> +AT_CHECK([perl $srcdir/uuidfilt.pl output], [0],
> +  [[[{"uuid":["uuid","<0>"]}]
> +[{"uuid":["uuid","<1>"]}]
> +[{"rows":[{"_uuid":["uuid","<0>"],"_version":["uuid","<2>"],"name":"zero","number":0},{"_uuid":["uuid","<1>"],"_version":["uuid","<3>"],"name":"one","number":1}]}]
> +]], [],
> +         [test ! -e pid || kill `cat pid`])
> +AT_CLEANUP
> +
>  AT_SETUP([ovsdb-client get-schema-version])
>  AT_KEYWORDS([ovsdb server positive])
>  AT_DATA([schema], [ORDINAL_SCHEMA
> --
> 1.7.1
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>



More information about the dev mailing list