GNOME.org
 

Re: [gnome-db] A few bug fix in mysql provider

> > +	/* FIXME: The prototype is:           gda_value_free (GdaValue*)
> > +	          but it is called this way:  gda_value_free (GdaValue*, (gpointer) NULL)
> > +	          Is this safe?  Doesn't this write NULL other object's memory?
> > +	*/
> >
> remove this comment, please.

Removed.  I didn't know enough about compiler internal working and was
paranoid of buffer overflow when I saw the type mismatch.


> also, here, just remove the function, no need to add those comments,
> which just make the code more confusing.
> ...
> the same here, please just remove code when it's no longer needed, dont
> comment it out.
> ... 
> here, again, what's this comment about? If it's wrong, it shouldn't be
> in the code at all.

Removed.  It's a coding style left over from non CVS project.  Sorry
for the annoyance, and watch out for more CVS newbies mistakes coming
your way :-).


> > gda_field_attributes_set_defined_size (attrs, mysql_field->length);
> > gda_field_attributes_set_table (attrs, mysql_field->table);
> > /* gda_field_attributes_set_caption(attrs, ); */
> > gda_field_attributes_set_scale (attrs, mysql_field->decimals);
> > gda_field_attributes_set_gdatype (attrs, gda_mysql_type_to_gda (mysql_field->type));
> > gda_field_attributes_set_allow_null (attrs, !IS_NOT_NULL (mysql_field->flags));
> > gda_field_attributes_set_primary_key (attrs, IS_PRI_KEY (mysql_field->flags));
> > gda_field_attributes_set_unique_key (attrs, mysql_field->flags & UNIQUE_KEY_FLAG);
> > /* gda_field_attributes_set_references(attrs, ); */
> > gda_field_attributes_set_auto_increment (attrs, mysql_field->flags & AUTO_INCREMENT_FLAG);
> > /* attrs->auto_increment_start */
> > /* attrs->auto_increment_step  */
> > gda_field_attributes_set_position (attrs, col);

> here again, if its wrong, remove it, or just fill in the missing data.

Here we are initializing most members of a big struct.  The comments
help there to remind us that other fields are intentially left
untouched--not that they are forgotten or the code is out of sync with
struct's definition.  I consider this a good programing style.  Please
feel free to remove them if you disagree.  You make the call.


> > +	/* FIXME: 
> > +	   Can't use mysql_field->def because,
> > +	     - don't know length of the mysql_field->def (needed for binary data)
> > +	     - manual says we mysql_field->def isn't set unless we use
> > +	       mysql_list_fields().  Then it recommend not to use this function,
> > +	       but instead use "show columns from table_name;".
> > +
> > +	       this is same as "describe table_name;" ?
> > +	gda_field_attributes_set_default_value (attrs, ...);
> > +	*/
> >  
> so, you dont set the default value? Then, please add the code to handle
> it and send a different patch for it. This patch was about fixing a
> leak, and it contains much more things :-) So, please, send a patch for
> the leak, and then, when done, a different patch for the default value
> implementation.
> 
> But adding commented/unfinished code is not an option :-)

It contains more than bug fixes because I started off in CVS HEAD
mindset, and just happened to discover bugs as I go on.  Anyway, a
patch with no mention of default_value follows.

Some "FIXME" comments are left in the patch, pointing to bugs that
exist in the original code.  Ideally, I should fix these bugs.  But
just adding warning comments for now is better than doing nothing with
them ;-).

Paisa




? doc/C/tmpl/gda-enum-types.sgml
? libgda/gda-enum-types.c
? libgda/gda-enum-types.h
? libgda/s-enum-types-c
? libgda/s-enum-types-h
Index: ChangeLog
===================================================================
RCS file: /cvs/gnome/libgda/ChangeLog,v
retrieving revision 1.575
diff -u -r1.575 ChangeLog
--- ChangeLog	13 Oct 2003 03:19:20 -0000	1.575
+++ ChangeLog	15 Oct 2003 00:37:10 -0000
@@ -1,3 +1,15 @@
+2003-10-13  Paisa Seeluangsawat <paisa users sf net>
+
+	* providers/mysql/gda-mysql-provider.c: 
+	  - add #include <libgda/gda-util.h> 
+	  - minor cosmatic fixes to stop some compiler warnings.
+	* providers/mysql/gda-mysql-recordset.c
+	  - fixed possible memory leak by moving gda_row_new() a few line down.
+	  - commented out variable "lengths", which doesn't seem to be used
+	    anywhere.
+	  - handle FIELD_TYPE_BLOB length correctly
+	  - distinguish SQL NULL from 0, 0.0, "".
+
 2003-10-12  Paisa Seeluangsawat <paisa users sf net>
 
 	* libgda/gda-field.h:
Index: doc/C/tmpl/gda-blob.sgml
===================================================================
RCS file: /cvs/gnome/libgda/doc/C/tmpl/gda-blob.sgml,v
retrieving revision 1.2
diff -u -r1.2 gda-blob.sgml
--- doc/C/tmpl/gda-blob.sgml	11 Oct 2003 15:15:11 -0000	1.2
+++ doc/C/tmpl/gda-blob.sgml	15 Oct 2003 00:37:10 -0000
@@ -9,12 +9,10 @@
 
 </para>
 
-
 <!-- ##### SECTION See_Also ##### -->
 <para>
 
 </para>
-
 
 <!-- ##### ENUM GdaBlobMode ##### -->
 <para>
Index: providers/mysql/gda-mysql-provider.c
===================================================================
RCS file: /cvs/gnome/libgda/providers/mysql/gda-mysql-provider.c,v
retrieving revision 1.45
diff -u -r1.45 gda-mysql-provider.c
--- providers/mysql/gda-mysql-provider.c	15 Aug 2003 12:53:40 -0000	1.45
+++ providers/mysql/gda-mysql-provider.c	15 Oct 2003 00:37:11 -0000
@@ -23,6 +23,7 @@
  */
 
 #include <libgda/gda-data-model-array.h>
+#include <libgda/gda-util.h>
 #include <libgda/gda-intl.h>
 #include <stdlib.h>
 #include <string.h>
@@ -350,8 +351,8 @@
 			mysql_res = mysql_store_result (mysql);
 			recset = gda_mysql_recordset_new (cnc, mysql_res, mysql);
 			if (GDA_IS_MYSQL_RECORDSET (recset)) {
-				gda_data_model_set_command_text (recset, arr[n]);
-				gda_data_model_set_command_type (recset, GDA_COMMAND_TYPE_SQL);
+				gda_data_model_set_command_text ( (GdaDataModel*) recset, arr[n]);
+				gda_data_model_set_command_type ( (GdaDataModel*) recset, GDA_COMMAND_TYPE_SQL);
 				reclist = g_list_append (reclist, recset);
 			}
 
@@ -663,7 +664,6 @@
 static void
 add_aggregate_row (GdaDataModelArray *recset, const gchar *str, const gchar *comments)
 {
-	GdaValue *value;
 	GList *list;
 
 	g_return_if_fail (GDA_IS_DATA_MODEL_ARRAY (recset));
@@ -870,7 +870,7 @@
 {
 	GList *reclist;
 	GdaMysqlRecordset *recset;
-	GdaDataModelArray *model;
+	GdaDataModel *model;
 	gint rows, r;
 
 	g_return_val_if_fail (GDA_IS_CONNECTION (cnc), NULL);
@@ -886,10 +886,10 @@
 
 	/* add the extra information */
 	model = gda_data_model_array_new (4);
-	gda_data_model_set_column_title (GDA_DATA_MODEL (model), 0, _("Name"));
-	gda_data_model_set_column_title (GDA_DATA_MODEL (model), 1, _("Owner"));
-	gda_data_model_set_column_title (GDA_DATA_MODEL (model), 2, _("Comments"));
-	gda_data_model_set_column_title (GDA_DATA_MODEL (model), 3, "SQL");
+	gda_data_model_set_column_title (model, 0, _("Name"));
+	gda_data_model_set_column_title (model, 1, _("Owner"));
+	gda_data_model_set_column_title (model, 2, _("Comments"));
+	gda_data_model_set_column_title (model, 3, "SQL");
 	rows = gda_data_model_get_n_rows (GDA_DATA_MODEL (recset));
 	for (r = 0; r < rows; r++) {
 		GList *value_list = NULL;
@@ -924,14 +924,14 @@
 		else
 			value_list = g_list_append (value_list, gda_value_new_string (""));
 
-		gda_data_model_append_row (GDA_DATA_MODEL (model), value_list);
+		gda_data_model_append_row (model, value_list);
 
 		g_free (name);
 		g_list_foreach (value_list, (GFunc) gda_value_free, NULL);
 		g_list_free (value_list);
 	}
 
-	return GDA_DATA_MODEL (model);
+	return model;
 }
 
 static GdaDataModel *
Index: providers/mysql/gda-mysql-recordset.c
===================================================================
RCS file: /cvs/gnome/libgda/providers/mysql/gda-mysql-recordset.c,v
retrieving revision 1.26
diff -u -r1.26 gda-mysql-recordset.c
--- providers/mysql/gda-mysql-recordset.c	24 Aug 2003 12:07:16 -0000	1.26
+++ providers/mysql/gda-mysql-recordset.c	15 Oct 2003 00:37:11 -0000
@@ -41,6 +41,66 @@
  * Private functions
  */
 
+void
+fill_gda_value (GdaValue *gda_value, enum enum_field_types type, gchar *value, unsigned long length)
+{
+	if (!value) {
+		gda_value_set_null (gda_value);
+		return;
+	}
+
+	switch (type) {
+	case FIELD_TYPE_DECIMAL :
+	case FIELD_TYPE_DOUBLE :
+		gda_value_set_double (gda_value, atof (value));
+		break;
+	case FIELD_TYPE_FLOAT :
+		gda_value_set_single (gda_value, atof (value));
+		break;
+	case FIELD_TYPE_LONG :
+	case FIELD_TYPE_YEAR :
+		gda_value_set_integer (gda_value, atol (value));
+		break;
+	case FIELD_TYPE_LONGLONG :
+	case FIELD_TYPE_INT24 :
+		gda_value_set_bigint (gda_value, atoll (value));
+		break;
+	case FIELD_TYPE_SHORT :
+		gda_value_set_smallint (gda_value, atoi (value));
+		break;
+	case FIELD_TYPE_TINY :
+		gda_value_set_tinyint (gda_value, atoi (value));
+		break;
+	case FIELD_TYPE_TINY_BLOB :
+	case FIELD_TYPE_MEDIUM_BLOB :
+	case FIELD_TYPE_LONG_BLOB :
+	case FIELD_TYPE_BLOB :
+		gda_value_set_binary (gda_value, value, length);
+		break;
+	case FIELD_TYPE_VAR_STRING :
+	case FIELD_TYPE_STRING :
+		/* FIXME: we might get "[VAR]CHAR(20) BINARY" type with \0 inside
+		   We should check for BINARY flag and treat it like a BLOB
+		 */
+		gda_value_set_string (gda_value, value);
+		break;
+	case FIELD_TYPE_DATE :
+	case FIELD_TYPE_NULL :
+	case FIELD_TYPE_NEWDATE :
+	case FIELD_TYPE_ENUM :
+	case FIELD_TYPE_TIMESTAMP :
+	case FIELD_TYPE_DATETIME :
+	case FIELD_TYPE_TIME :
+	case FIELD_TYPE_SET : /* FIXME */
+		gda_value_set_string (gda_value, value);
+		break;
+	default :
+		g_printerr ("Unknown MySQL datatype.  This is fishy, but continueing anyway.\n");
+		gda_value_set_string (gda_value, value);
+	}
+}
+
+
 static GdaRow *
 fetch_row (GdaMysqlRecordset *recset, gulong rownum)
 {
@@ -48,9 +108,10 @@
 	gint field_count;
 	gint row_count;
 	gint i;
-	unsigned long *lengths;
 	MYSQL_FIELD *mysql_fields;
 	MYSQL_ROW mysql_row;
+	unsigned long *mysql_lengths;
+
 
 	g_return_val_if_fail (GDA_IS_MYSQL_RECORDSET (recset), NULL);
 
@@ -71,68 +132,19 @@
 	}
 
 	mysql_data_seek (recset->mysql_res, rownum);
-	row = gda_row_new (GDA_DATA_MODEL (recset), field_count);
-
-	lengths = recset->mysql_res->lengths;
 	mysql_fields = mysql_fetch_fields (recset->mysql_res);
-
 	mysql_row = mysql_fetch_row (recset->mysql_res);
-	if (!mysql_row)
+	mysql_lengths = mysql_fetch_lengths (recset->mysql_res);
+	if (!mysql_row || !mysql_lengths)
 		return NULL;
+	
+	row = gda_row_new (GDA_DATA_MODEL (recset), field_count);
 
 	for (i = 0; i < field_count; i++) {
-		GdaValue *field;
-		gchar *thevalue;
-
-		field = (GdaValue*) gda_row_get_value (row, i);
-
-		thevalue = mysql_row[i];
-
-		switch (mysql_fields[i].type) {
-		case FIELD_TYPE_DECIMAL :
-		case FIELD_TYPE_DOUBLE :
-			gda_value_set_double (field, thevalue ? atof (thevalue) : 0.0);
-			break;
-		case FIELD_TYPE_FLOAT :
-			gda_value_set_single (field, thevalue ? atof (thevalue) : 0.0);
-			break;
-		case FIELD_TYPE_LONG :
-		case FIELD_TYPE_YEAR :
-			gda_value_set_integer (field, thevalue ? atol (thevalue) : 0);
-			break;
-		case FIELD_TYPE_LONGLONG :
-		case FIELD_TYPE_INT24 :
-			gda_value_set_bigint (field, thevalue ? atoll (thevalue) : 0);
-			break;
-		case FIELD_TYPE_SHORT :
-			gda_value_set_smallint (field, thevalue ? atoi (thevalue) : 0);
-			break;
-		case FIELD_TYPE_TINY :
-			gda_value_set_tinyint (field, thevalue ? atoi (thevalue) : 0);
-			break;
-		case FIELD_TYPE_TINY_BLOB :
-		case FIELD_TYPE_MEDIUM_BLOB :
-		case FIELD_TYPE_LONG_BLOB :
-		case FIELD_TYPE_BLOB :
-			gda_value_set_binary (field, thevalue, mysql_fields[i].max_length);
-			break;
-		case FIELD_TYPE_VAR_STRING :
-		case FIELD_TYPE_STRING :
-			gda_value_set_string (field, thevalue ? thevalue : "");
-			break;
-		case FIELD_TYPE_DATE :
-		case FIELD_TYPE_NULL :
-		case FIELD_TYPE_NEWDATE :
-		case FIELD_TYPE_ENUM :
-		case FIELD_TYPE_TIMESTAMP :
-		case FIELD_TYPE_DATETIME :
-		case FIELD_TYPE_TIME :
-		case FIELD_TYPE_SET : /* FIXME */
-			gda_value_set_string (field, thevalue ? thevalue : "");
-			break;
-		default :
-			gda_value_set_string (field, thevalue ? thevalue : "");
-		}
+		fill_gda_value ((GdaValue*) gda_row_get_value (row, i),
+			    mysql_fields[i].type,
+			    mysql_row[i],
+			    mysql_lengths[i]);
 	}
 
 	return row;
@@ -186,24 +198,27 @@
 	if (col >= field_count)
 		return NULL;
 
-	attrs = gda_field_attributes_new ();
-
 	mysql_field = mysql_fetch_field_direct (recset->mysql_res, col);
-	if (!mysql_field) {
-		gda_field_attributes_free (attrs);
+	if (!mysql_field)
 		return NULL;
-	}
+
+	attrs = gda_field_attributes_new ();
 
 	if (mysql_field->name)
 		gda_field_attributes_set_name (attrs, mysql_field->name);
 	gda_field_attributes_set_defined_size (attrs, mysql_field->length);
 	gda_field_attributes_set_table (attrs, mysql_field->table);
+	/* gda_field_attributes_set_caption(attrs, ); */
 	gda_field_attributes_set_scale (attrs, mysql_field->decimals);
 	gda_field_attributes_set_gdatype (attrs, gda_mysql_type_to_gda (mysql_field->type));
 	gda_field_attributes_set_allow_null (attrs, !IS_NOT_NULL (mysql_field->flags));
 	gda_field_attributes_set_primary_key (attrs, IS_PRI_KEY (mysql_field->flags));
 	gda_field_attributes_set_unique_key (attrs, mysql_field->flags & UNIQUE_KEY_FLAG);
+	/* gda_field_attributes_set_references(attrs, ); */
 	gda_field_attributes_set_auto_increment (attrs, mysql_field->flags & AUTO_INCREMENT_FLAG);
+	/* attrs->auto_increment_start */
+	/* attrs->auto_increment_step  */
+	gda_field_attributes_set_position (attrs, col);
 
 	return attrs;
 }
Index: providers/mysql/utils.c
===================================================================
RCS file: /cvs/gnome/libgda/providers/mysql/utils.c,v
retrieving revision 1.6
diff -u -r1.6 utils.c
--- providers/mysql/utils.c	19 May 2002 18:55:52 -0000	1.6
+++ providers/mysql/utils.c	15 Oct 2003 00:37:11 -0000
@@ -78,6 +78,7 @@
 		return GDA_VALUE_TYPE_BINARY;
 	case FIELD_TYPE_VAR_STRING :
 	case FIELD_TYPE_STRING :
+		/* FIXME: Check for BINARY flags and use blob */
 		return GDA_VALUE_TYPE_STRING;
 	case FIELD_TYPE_NULL :
 	case FIELD_TYPE_NEWDATE :
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]