Re: Patches

Nirgal Vourgère <[email protected]> Fri, 18 Feb 2011 12:47:40 +0100

Newsgroups	gmane.comp.db.mdb-tools.devel
Message-ID	<[email protected]>

On Thursday 17 February 2011 00:23:45 Brian Bruns wrote:
> All patches applied to master.

Great :)
I've been posting these patches on the list, but the post broke the 40KB message limit.

> Am I correct in surmising that the
> git.diff patch brings the code from 0.6pre1 to the current (prior to
> the other patches) git repository level?

You were absolutly right to ignore that one.
This is because I'm using Debian packaging and it always use the last original tar as a reference.

> I have noticed a couple of problems after applying the patches.
> First, the 'ole' patch changes mdb-export.c/print_col()
> -        if (strlen(quote_char)==1 && c==quote_char[0]) {
> +        if (strlen(quote_char)==1 && c==quote_char[0] || c==escape_char[0]) {
> But if -X is not passed to mdb-export, then escape_char is NULL and we
> segfault on the above code.  Possibly a misapplied patch, but can you
> check it on your local copy?

Yes, this is a bug I introduced, trying to escape both the the escape char and the quote char, rather than only the quote char. I think we should escape the escape char. What is your opinion?
Attached is a fix patch.
I took the liberty to reorganize things a bit:
quote_char and escape_char are now just of type char rather than strings. It simplifies thing a lot.
I also added warning in option parsing, when quote char and escape char lengths are <>1. They were ignoring then.

I submited to the list only a tiny fraction of the patches I've been writing for that small print_col function.
I've been confused by the fact that print_col is used both for INSERT and CSV.
I'm afraid this is just not possible.
Right now, I think CSV is working ok. But INSERT is broken for ole/binary values, and probably for control characters (<32) too.
I think we need some dialect specific code for INSERT, since each backend migth escape the strings & blob in their own way. IMHO, we need a function similar to MdbBackend->quote_schema_name. Eg postgresql E'...' and X'...' formats.

Attached is "export" diff file anyways.

> Secondly, I am getting a message of 'Warning: incorrect memo length'
> on a database that previously did not emit that error.  I need to
> check into this one a bit more.

That problem was because I was trying to handle MDB_BINARY type like MDB_MEMO. This was wrong, especially because there is no unicode encoding. We need to handle them more like OLE I guess.

Note that I can see a problem with memo length computation, just like the OLE length computation was broken, with the 4 extra bytes of the chunk length being taken into account.

Attached is "binaries" diff file.

Another note: I've been working with a JET3 chinese database using BIG5 charset, and I had quite a lot of bad surprises with buffer sizes, with unicode2ascii that can triple the size of data. We potentially have overflows there.

-- 
- - Nirgal Vourgère ☮ GPI IT ☮ tel:+33.1.44.64.02.96
-   xmpp:[email protected] ☮ gpg 0x4760b41db292ab13 ☮ skype:nirgal_v
    nirgal.com:qotd ☮ Be the change you want to see in the world -- Gandhi

------------------------------------------------------------------------------
The ultimate all-in-one performance toolkit: Intel(R) Parallel Studio XE:
Pinpoint memory and threading errors before they happen.
Find and fix more than 250 security defects in the development cycle.
Locate bottlenecks in serial and parallel code that limit performance.
http://p.sf.net/sfu/intel-dev2devfeb

_______________________________________________
mdbtools-dev mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/mdbtools-dev

export (text/x-patch, 4.2 KB)

Index: mdbtools-0.6pre1/src/util/mdb-export.c
===================================================================
--- mdbtools-0.6pre1.orig/src/util/mdb-export.c
+++ mdbtools-0.6pre1/src/util/mdb-export.c
@@ -26,41 +26,35 @@
 #undef MDB_BIND_SIZE
 #define MDB_BIND_SIZE 200000
 
-#define is_text_type(x) (x==MDB_TEXT || x==MDB_OLE || x==MDB_MEMO || x==MDB_DATETIME || x==MDB_BINARY)
+#define is_quote_type(x) (x==MDB_TEXT || x==MDB_OLE || x==MDB_MEMO || x==MDB_DATETIME || x==MDB_BINARY)
+#define is_binary_type(x) (x==MDB_OLE || x==MDB_BINARY)
 
 static char *escapes(char *s);
 
-void
-print_col(gchar *col_val, int quote_text, int col_type, int bin_length, char *quote_char, char *escape_char)
+static void
+print_col(gchar *col_val, int quote_text, int col_type, int bin_length, char quote_char, char escape_char)
 {
-	gchar *s;
-	unsigned char c;
-
-	if (quote_text && is_text_type(col_type)) {
-		fputs(quote_char,stdout);
-		if (col_type == MDB_OLE || col_type == MDB_BINARY)	{
+	gchar c;
+	if (!escape_char)
+		/* double the quote char if no escape char passed */
+		escape_char = quote_char;
+
+	if (quote_text && is_quote_type(col_type)) {
+		fputc(quote_char,stdout);
+		if (is_binary_type(col_type))
 			while (bin_length--) {
-				c = (unsigned char)*col_val++;
-				if (strlen(quote_char)==1 && c==quote_char[0] || c==escape_char[0]) {
-					if (escape_char)
-						fputs(escape_char,stdout);
-					else /* double the quote char if no escape char passed */
-						fputs(quote_char,stdout);
-				}
+				c = *col_val++;
+				if (c==quote_char || c==escape_char)
+					fputc(escape_char,stdout);
 				putc(c, stdout);
 			}
-		}
 		else
-			for (s=col_val;(c=*s);s++) {
-				if (strlen(quote_char)==1 && c==quote_char[0] || c==escape_char[0]) {
-					if (escape_char)
-						fputs(escape_char,stdout);
-					else /* double the quote char if no escape char passed */
-						fputs(quote_char,stdout);
-				}
+			while ((c=*col_val++)) {
+				if (c==quote_char || c==escape_char)
+					fputc(escape_char,stdout);
 				putc(c, stdout);
 			}
-		fputs(quote_char,stdout);
+		fputc(quote_char,stdout);
 	} else
 		fputs(col_val,stdout);
 }
@@ -75,8 +69,8 @@
 	int  *bound_lens; 
 	char *delimiter = NULL;
 	char *row_delimiter = NULL;
-	char *quote_char = NULL;
-	char *escape_char = NULL;
+	char quote_char = '"';
+	char escape_char = '\0';
 	char header_row = 1;
 	char quote_text = 1;
 	char *insert_dialect = NULL;
@@ -95,7 +89,9 @@
 			quote_text = 0;
 		break;
 		case 'q':
-			quote_char = (char *) g_strdup(optarg);
+			if (strlen(optarg)!=1)
+				fprintf(stderr, "-q parameter should be one char long.\n");
+			quote_char = optarg[0];
 		break;
 		case 'd':
 			delimiter = escapes(optarg);
@@ -114,7 +110,9 @@
 			mdb_set_date_fmt(optarg);
 		break;
 		case 'X':
-			escape_char = (char *) g_strdup(optarg);
+			if (strlen(optarg)!=1)
+				fprintf(stderr, "-X parameter should be one char long.\n");
+			escape_char = optarg[0];
 		break;
 		case 'N':
 			namespace = (char *) g_strdup(optarg);
@@ -123,9 +121,6 @@
 		break;
 		}
 	}
-	if (!quote_char) {
-		quote_char = (char *) g_strdup("\"");
-	}
 	if (!delimiter) {
 		delimiter = (char *) g_strdup(",");
 	}
@@ -152,8 +147,6 @@
 		fprintf(stderr,"  -N <namespace> Prefix identifiers with namespace\n");
 		g_free (delimiter);
 		g_free (row_delimiter);
-		g_free (quote_char);
-		if (escape_char) g_free (escape_char);
 		exit(1);
 	}
 
@@ -162,8 +155,6 @@
 	if (!(mdb = mdb_open(argv[optind], MDB_NOFLAGS))) {
 		g_free (delimiter);
 		g_free (row_delimiter);
-		g_free (quote_char);
-		if (escape_char) g_free (escape_char);
 		mdb_exit();
 		exit(1);
 	}
@@ -171,7 +162,6 @@
 	if (insert_dialect)
 		if (!mdb_set_default_backend(mdb, insert_dialect)) {
 			fprintf(stderr, "Invalid backend type\n");
-			if (escape_char) g_free (escape_char);
 			mdb_exit();
 			exit(1);
 		}
@@ -181,8 +171,6 @@
 		fprintf(stderr, "Error: Table %s does not exist in this database.\n", argv[argc-1]);
 		g_free (delimiter);
 		g_free (row_delimiter);
-		g_free (quote_char);
-		if (escape_char) g_free (escape_char);
 		mdb_close(mdb);
 		mdb_exit();
 		exit(1);
@@ -261,8 +249,6 @@
 
 	g_free (delimiter);
 	g_free (row_delimiter);
-	g_free (quote_char);
-	if (escape_char) g_free (escape_char);
 	mdb_close(mdb);
 	mdb_exit();

binaries (text/x-patch, 1.4 KB)

Index: mdbtools-0.6pre1/src/libmdb/data.c
===================================================================
--- mdbtools-0.6pre1.orig/src/libmdb/data.c
+++ mdbtools-0.6pre1/src/libmdb/data.c
@@ -308,7 +308,7 @@
 		mdb_xfer_bound_bool(mdb, col, isnull);
 	} else if (isnull) {
 		mdb_xfer_bound_data(mdb, 0, col, 0);
-	} else if (col->col_type == MDB_OLE) {
+	} else if (col->col_type == MDB_OLE || col->col_type == MDB_BINARY) {
 		mdb_xfer_bound_ole(mdb, offset, col, len);
 	} else {
 		//if (!mdb_test_sargs(mdb, col, offset, len)) {
@@ -927,7 +927,6 @@
 			text = mdb_date_to_string(mdb, start);
 		break;
 		case MDB_MEMO:
-		case MDB_BINARY:
 			text = mdb_memo_to_string(mdb, start, size);
 		break;
 		case MDB_MONEY:
Index: mdbtools-0.6pre1/src/util/mdb-export.c
===================================================================
--- mdbtools-0.6pre1.orig/src/util/mdb-export.c
+++ mdbtools-0.6pre1/src/util/mdb-export.c
@@ -226,14 +226,14 @@
 				if (insert_dialect)
 					fputs("NULL", stdout);
 			} else {
-				if (col->col_type == MDB_OLE) {
+				if (is_binary_type(col->col_type)) {
 					value = mdb_ole_read_full(mdb, col, &length);
 				} else {
 					value = bound_values[j];
 					length = bound_lens[j];
 				}
 				print_col(value, quote_text, col->col_type, length, quote_char, escape_char);
-				if (col->col_type == MDB_OLE)
+				if (is_binary_type(col->col_type))
 					free(value);
 			}
 		}

signature.asc (application/pgp-signature, 198 B)

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)

iEYEABECAAYFAk1eXF4ACgkQR2C0HbKSqxOu0ACguFxBJ6wDGUX+DTgcNjEQLAQm
b7wAoIN04thOeaWDSM6kR9ZGOVjoccJa
=A37q
-----END PGP SIGNATURE-----