diff mbox

Change in openbsc[master]: db.c: implemented incremental migration

Message ID gerrit.1463132482595.Ia9c2aa86f96b88ad8a710d0a23879ce219bc82dc@gerrit.osmocom.org
State New
Headers show

Commit Message

gerrit-no-reply@lists.osmocom.org May 13, 2016, 9:41 a.m. UTC 
From Vadim Yanitskiy <axilirator@gmail.com>:

Vadim Yanitskiy has uploaded a new change for review.

  https://gerrit.osmocom.org/62

Change subject: db.c: implemented incremental migration
......................................................................

db.c: implemented incremental migration

In the past, normal migration was possible only if the actual
schema version differed from the version used in DB by 1. For
example, if DB uses an old version 3 and you need to use it
with the code written for version 5, the check_db_revision()
will convert it to 4 and DB will still use incompatible schema
version during Osmo-NITB running time. After next run it will
be converted to version 5.

This patch replaces a set of 'else-if' checks by a 'switch'
without 'break' statements between 'case' labels (waterfall).
It makes you able to migrate from current version to the
latest despite any difference between them.

Change-Id: Ia9c2aa86f96b88ad8a710d0a23879ce219bc82dc
---
M openbsc/src/libmsc/db.c
1 file changed, 35 insertions(+), 18 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/openbsc refs/changes/62/62/1

Comments

gerrit-no-reply@lists.osmocom.org May 13, 2016, 10:19 a.m. UTC | #1 
From Holger Freyther <holger@freyther.de>:

Holger Freyther has posted comments on this change.

Change subject: db.c: implemented incremental migration
......................................................................


Patch Set 1:

(2 comments)

Could you please add a test db with version 1 preferable and "test" the migration? IIRC we already one case for the SMS conversion I did a couple of years ago.

https://gerrit.osmocom.org/#/c/62/1/openbsc/src/libmsc/db.c
File openbsc/src/libmsc/db.c:

Line 412: 			goto error;
For coverity we will most likely need to add a "fallthrough" comment or such.


PS1, Line 418: break
indent
gerrit-no-reply@lists.osmocom.org May 13, 2016, 10:38 a.m. UTC | #2 
From Vadim Yanitskiy <axilirator@gmail.com>:

Vadim Yanitskiy has posted comments on this change.

Change subject: db.c: implemented incremental migration
......................................................................


Patch Set 1:

(1 comment)

> (2 comments)
 > 
 > Could you please add a test db with version 1 preferable and "test"
 > the migration? IIRC we already one case for the SMS conversion I
 > did a couple of years ago.

I have already tested migration process from 3 to 5.
Maybe it would be better to do in another commit?
We can name it "db.c: implemented update_db_revision_1()" or such.
What do you thik about it?

https://gerrit.osmocom.org/#/c/62/1/openbsc/src/libmsc/db.c
File openbsc/src/libmsc/db.c:

Line 412: 			goto error;
> For coverity we will most likely need to add a "fallthrough" comment or suc
Should I add this comment before every 'case' statement?
Or just replace the 'waterfall' by 'fallthrough'?
diff mbox

Patch

diff --git a/openbsc/src/libmsc/db.c b/openbsc/src/libmsc/db.c
index e5017ae..b3235bb 100644
--- a/openbsc/src/libmsc/db.c
+++ b/openbsc/src/libmsc/db.c
@@ -374,9 +374,13 @@ 
 {
 	dbi_result result;
 	const char *rev_s;
+	int db_rev = 0;
 
+	/* Make a query */
 	result = dbi_conn_query(conn,
-				"SELECT value FROM Meta WHERE key='revision'");
+		"SELECT value FROM Meta "
+		"WHERE key = 'revision'");
+
 	if (!result)
 		return -EINVAL;
 
@@ -384,33 +388,46 @@ 
 		dbi_result_free(result);
 		return -EINVAL;
 	}
+
+	/* Fetch the DB schema revision */
 	rev_s = dbi_result_get_string(result, "value");
 	if (!rev_s) {
 		dbi_result_free(result);
 		return -EINVAL;
 	}
-	if (!strcmp(rev_s, "2")) {
-		if (update_db_revision_2()) {
-			LOGP(DDB, LOGL_FATAL, "Failed to update database from schema revision '%s'.\n", rev_s);
-			dbi_result_free(result);
-			return -EINVAL;
-		}
-	} else if (!strcmp(rev_s, "3")) {
-		if (update_db_revision_3()) {
-			LOGP(DDB, LOGL_FATAL, "Failed to update database from schema revision '%s'.\n", rev_s);
-			dbi_result_free(result);
-			return -EINVAL;
-		}
-	} else if (!strcmp(rev_s, SCHEMA_REVISION)) {
-		/* everything is fine */
-	} else {
-		LOGP(DDB, LOGL_FATAL, "Invalid database schema revision '%s'.\n", rev_s);
+
+	if (!strcmp(rev_s, SCHEMA_REVISION)) {
+		/* Everything is fine */
 		dbi_result_free(result);
+		return 0;
+	}
+
+	db_rev = atoi(rev_s);
+	dbi_result_free(result);
+
+	/* Incremental migration waterfall */
+	switch (db_rev) {
+	case 2:
+		if (update_db_revision_2())
+			goto error;
+	case 3:
+		if (update_db_revision_3())
+			goto error;
+
+	/* The end of waterfall */
+	break;
+	default:
+		LOGP(DDB, LOGL_FATAL,
+			"Invalid database schema revision '%d'.\n", db_rev);
 		return -EINVAL;
 	}
 
-	dbi_result_free(result);
 	return 0;
+
+error:
+	LOGP(DDB, LOGL_FATAL, "Failed to update database "
+		"from schema revision '%d'.\n", db_rev);
+	return -EINVAL;
 }
 
 static int db_configure(void)