Looks Good.
I didn't closely review the tests.
Ethan
On Mon, Mar 28, 2011 at 1:07 PM, Ben Pfaff <b...@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@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
