On 8/18/25 11:46 PM, Mike Pattrick via dev wrote: > Previously ovs-lib would assume a database is valid if the file exists, > however, it is possible for the database file to exist but not be valid > for ovsdb to open it. > > Now existence checks are augmented with empty file validation. > Empy databases are removed.
*Empty > > Reported-at: https://issues.redhat.com/browse/FDP-689 > Reported-by: Ihar Hrachyshka > Signed-off-by: Mike Pattrick <[email protected]> > > --- > v2: > - Back up database before deleting it > - Use the db-name command to check for validity of file > - Added test to verify that valid clustered databases are detected as > v3: > - Removed sourcing of lsb functions > - Corrected comment formatting. > - Restored test instead of valdidate_db check > v4: > - Simplify database validation > > Signed-off-by: Mike Pattrick <[email protected]> > --- > tests/ovsdb-server.at | 63 +++++++++++++++++++++++++++++++++++++++++++ > utilities/ovs-lib.in | 31 ++++++++++++++++++--- > 2 files changed, 91 insertions(+), 3 deletions(-) > > diff --git a/tests/ovsdb-server.at b/tests/ovsdb-server.at > index ca6e931be..9ce33af74 100644 > --- a/tests/ovsdb-server.at > +++ b/tests/ovsdb-server.at > @@ -105,6 +105,69 @@ AT_CHECK([uuidfilt output], [0], > ]], []) > AT_CLEANUP > > +AT_SETUP([invalid database is recreated]) > +AT_KEYWORDS([ovsdb unix]) > +AT_SKIP_IF([test "$IS_WIN32" = "yes"]) > +ordinal_schema > schema > + > +. ovs-lib > + > +dnl Check that DB is recreated when file is empty. > +truncate -s 0 db > +AT_CHECK([upgrade_db db schema], [0], [stdout], [ignore]) > +AT_CHECK([grep -c "db does not exist\|Creating empty database db" stdout], > [0], [2 > +]) > +AT_CHECK([validate_db db], [0]) > + > +AT_DATA([txnfile], [[ovsdb-client transact unix:socket \ > +'["ordinals", > + {"op": "insert", > + "table": "ordinals", > + "row": {"number": 1, "name": "one"}}]' > +]]) > +AT_CHECK([ovsdb-server --remote=punix:socket db --run="sh txnfile"], [0], > [stdout], [stderr]) > + > +dnl Check that DB is not recreated on corrupted log. This is similar to the > +dnl previous test but includes a mid-operation upgrade. > +echo 'xxx' >> db > +AT_CHECK([upgrade_db db schema], [0], [], [ignore]) > + > +dnl Validate that the db can now be used. > +AT_DATA([txnfile], [[ovsdb-client transact unix:socket \ > +'["ordinals", > + {"op": "select", > + "table": "ordinals", > + "where": []}]' > +]]) > +AT_CHECK([ovsdb-server --remote=punix:socket db --run="sh txnfile"], [0], > [stdout], [stderr]) > +AT_CHECK([grep -q 'syntax error: db: parse error.* in header line "xxx"' > stderr]) > +AT_CHECK([uuidfilt stdout], [0], > + > [[[{"rows":[{"_uuid":["uuid","<0>"],"_version":["uuid","<1>"],"name":"one","number":1}]}] > +]], []) > + > +dnl Validate then create and join cluster. Looks like the actual order below is opposite: create then validate. > +truncate -s 0 db > +AT_CHECK([create_cluster db schema tcp:1.1.1.1:1111 1000], [0], [stdout], []) > +AT_CHECK([grep -Ec 'Creating cluster database db' stdout], [0], [1 > +]) > +AT_CHECK([validate_db db]) > + > +dnl Corrupt a cluster db, validate does not clear it. > +echo 'x' >> db > +AT_CHECK([validate_db db]) > + > +dnl Invalidate the DB, validate_db does clear it. We clear it and the validate_db removes it. > +truncate -s 0 db I'd suggest wrapping all the 'truncate' and 'echo' calls into AT_CHECK, so they show up in the test log. > +AT_CHECK([validate_db db], [1], [ignore]) > + > +dnl Join a cluster with an empty db. > +truncate -s 0 db > +AT_CHECK([join_cluster db schema tcp:1.1.1.1:1111 tcp:2.2.2.2:2222], [0], > [stdout], []) > +AT_CHECK([grep -Ec 'Joining db to cluster' stdout], [0], [1 > +]) > +AT_CHECK([validate_db db]) > +AT_CLEANUP > + > AT_SETUP([truncating database log with bad transaction]) > AT_KEYWORDS([ovsdb server positive unix]) > AT_SKIP_IF([test "$IS_WIN32" = "yes"]) > diff --git a/utilities/ovs-lib.in b/utilities/ovs-lib.in > index dded0b7c7..4b92248dd 100644 > --- a/utilities/ovs-lib.in > +++ b/utilities/ovs-lib.in > @@ -445,16 +445,39 @@ create_db () { > > backup_db () { > # Back up the old version. > - version=`ovsdb_tool db-version "$DB_FILE"` > - cksum=`ovsdb_tool db-cksum "$DB_FILE" | awk '{print $1}'` > - backup=$DB_FILE.backup$version-$cksum > + if test ! -e "$DB_FILE"; then > + return 0 > + elif ovsdb_tool db-is-standalone "$DB_FILE" 2>/dev/null; then > + version=`ovsdb_tool db-version "$DB_FILE"` > + cksum=`ovsdb_tool db-cksum "$DB_FILE" | awk '{print $1}'` > + backup=$DB_FILE.backup$version-$cksum > + else > + # Support for clustered databases. > + backup=`mktemp -q $DB_FILE.XXXXXXXXXX` Do we actually want to suppress the failure diagnostic? The further actions will fail anyway if that happens, I suppose. > + fi > action "Backing up database to $backup" cp "$DB_FILE" "$backup" || > return 1 > } > > +validate_db () { > + # Returns 0 if $DB_FILE is present and at least has a db name. Outdated comments. > + # Returns 1 if $DB_FILE otherwise. This one reads funny. > + DB_FILE="$1" > + > + if test ! -e "$DB_FILE"; then > + return 1 > + elif [ $(wc -c < "$DB_FILE") -eq 0 ]; then It may be better to test for -s instead of using wc here. > + rm -f "$DB_FILE" > + return 1 > + fi > + > + return 0 > +} > + > upgrade_db () { > DB_FILE="$1" > DB_SCHEMA="$2" > > + validate_db "$DB_FILE" > schemaver=`ovsdb_tool schema-version "$DB_SCHEMA"` > if test ! -e "$DB_FILE"; then > log_warning_msg "$DB_FILE does not exist" > @@ -517,6 +540,7 @@ create_cluster () { > election_timer_arg="--election-timer=$ELECTION_TIMER_MS" > fi > > + validate_db "$DB_FILE" > if test ! -e "$DB_FILE"; then > action "Creating cluster database $DB_FILE" ovsdb_tool > $election_timer_arg create-cluster "$DB_FILE" "$DB_SCHEMA" "$LOCAL_ADDR" > elif ovsdb_tool db-is-standalone "$DB_FILE"; then > @@ -534,6 +558,7 @@ join_cluster() { > LOCAL_ADDR="$3" > REMOTE_ADDR="$4" > > + validate_db "$DB_FILE" > if test -e "$DB_FILE" && ovsdb_tool db-is-standalone "$DB_FILE"; then > backup_db || return 1 > rm $DB_FILE _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
