[GRASS5] [PATCHES]: pg.in.dbf

GRASS GRASS developer
1

Hello,

I have fixed several problems and made ---I think--- some improvements.
Some bugs were serious ones. In particular, the `init' function in
pgdump.c was not initializing correctly a structure causing random
data to be passed to PG.
I have not looked in the bug base, but these should close some bugs
referenced there.

Here is the ChangeLog :

main.c:
  - Added option to set the DELIMITER to a user defined character
  value (only used for super-user when using COPY command);
  -> Now PgDumpFromDBF takes 3 arguments: filename, mode and delimiter
  - PgDumpFromDBF proto removed from main.c and added to
  pgdump.h;
  - include <string.h> for strlen
  - include "pgdump.h" for PgDumpFromDBF proto
  - Default delimiter now changed to '\t';
  - Suppressed bogus parm structure ;
  - no_rattle -> normal_user: more explicit, match PgDumpFromDBF and
  ... well the way the assignement was made was queer: changed too!
  - Added comments

pgdump.h:
  - created for PgDumpFromDBF proto

pgdump.c:
  -XXX in the `init' function used to initialize the my_string struct
  the initial string pointed to by the data member was not initialized
  to NULL.
  Hence, the use of the function `append', not taking into account
  the length of the string and using `strcat' directly concatenated
  the random data found where my_string.data was pointing until it
  found some '\0'
  => Now my_string.data points to a null string at initialization;
  - include "pgdump.h" for PgDumpFromDBF proto
  - modified PgDumpFromDBF: now takes 4 arguments:
    filename, normal_user, delimiter, null_string
  - modified DBFDumpASCII: now takes 3 arguments:
    DBFHandle, File pointer, delimiter

-dbfopen.c:
  - the filename of the DBF file supposed to be opened was
  subject of extension changes, `strcmp' being used as if it was a
  matching function returning 0 if failed or not zero on success.
  And indeed this is almost exactly the contrary.
  Furthermore, the combination of two conditions made the first
  test succeeds in all cases, transforming every extension in ".dbf".

  -Last but not least, _without renaming or copying the file that
  was read not written_ this was this modified version of the
  filename that was given to `fopen' resulting in a
  failure in every case except when the change to ".dbf" was a
  null one, that is when the filename was already ending with ".dbf".

Cheers,
--
Thierry Laronde (Alceste) <tlaronde@polynum.org>
Key fingerprint = 0FF7 E906 FBAF FE95 FD89 250D 52B1 AE95 6006 F40C

(attachments)

pg.in.dbf.patch (13.5 KB)
pg.in.dbf.html.patch (2.7 KB)

2

In the absence of any comment from the original author I think we should
go ahead and apply Thierry's patches so they can be tested. The only thing
I would be concerned about is that there are maybe parts that should also
be applied to GRASS 5.7 if it perhaps shares some of this code? I am not
familiar with this part of GRASS so I can't comment.

I get a feeling the patches are a big improvement on the current pg.in.dbf
so I will go ahead and apply them in a few days if nobody objects.

Paul

On Sun, 12 Oct 2003, Thierry Laronde wrote:

Hello,

I have fixed several problems and made ---I think--- some improvements.
Some bugs were serious ones. In particular, the `init' function in
pgdump.c was not initializing correctly a structure causing random
data to be passed to PG.
I have not looked in the bug base, but these should close some bugs
referenced there.

Here is the ChangeLog :

main.c:
  - Added option to set the DELIMITER to a user defined character
  value (only used for super-user when using COPY command);
  -> Now PgDumpFromDBF takes 3 arguments: filename, mode and delimiter
  - PgDumpFromDBF proto removed from main.c and added to
  pgdump.h;
  - include <string.h> for strlen
  - include "pgdump.h" for PgDumpFromDBF proto
  - Default delimiter now changed to '\t';
  - Suppressed bogus parm structure ;
  - no_rattle -> normal_user: more explicit, match PgDumpFromDBF and
  ... well the way the assignement was made was queer: changed too!
  - Added comments

pgdump.h:
  - created for PgDumpFromDBF proto

pgdump.c:
  -XXX in the `init' function used to initialize the my_string struct
  the initial string pointed to by the data member was not initialized
  to NULL.
  Hence, the use of the function `append', not taking into account
  the length of the string and using `strcat' directly concatenated
  the random data found where my_string.data was pointing until it
  found some '\0'
  => Now my_string.data points to a null string at initialization;
  - include "pgdump.h" for PgDumpFromDBF proto
  - modified PgDumpFromDBF: now takes 4 arguments:
    filename, normal_user, delimiter, null_string
  - modified DBFDumpASCII: now takes 3 arguments:
    DBFHandle, File pointer, delimiter

-dbfopen.c:
  - the filename of the DBF file supposed to be opened was
  subject of extension changes, `strcmp' being used as if it was a
  matching function returning 0 if failed or not zero on success.
  And indeed this is almost exactly the contrary.
  Furthermore, the combination of two conditions made the first
  test succeeds in all cases, transforming every extension in ".dbf".

  -Last but not least, _without renaming or copying the file that
  was read not written_ this was this modified version of the
  filename that was given to `fopen' resulting in a
  failure in every case except when the change to ".dbf" was a
  null one, that is when the filename was already ending with ".dbf".

Cheers,
--
Thierry Laronde (Alceste) <tlaronde@polynum.org>
Key fingerprint = 0FF7 E906 FBAF FE95 FD89 250D 52B1 AE95 6006 F40C

3

Hello Paul,

On Mon, Oct 20, 2003 at 11:41:19AM +0100, Paul Kelly wrote:

In the absence of any comment from the original author I think we should
go ahead and apply Thierry's patches so they can be tested. The only thing
I would be concerned about is that there are maybe parts that should also
be applied to GRASS 5.7 if it perhaps shares some of this code? I am not
familiar with this part of GRASS so I can't comment.

The main things that may be worth looking for are the pieces of code
taken from Frank Warmerdam work (parts of gdal?) since there are some
bugs that may or may not be fixed in the newer versions, if they
were not introduced by the taker.
This has to be verified. I will give it a look this week-end.

The other thing is, IMHO, to avoid in the future compromises leading
to the use of parts of an alien library: whether we maintain our own
version, or use the vanilla one by linking against it. The main
advantage of a library is to be sure that all the programs use the
very same functions leading to the very same results: if something
has to be fixed, fix it once [in the library], and spread the benefits
everywhere [this library is in used].

Cheers,
--
Thierry Laronde (Alceste) <tlaronde@polynum.org>
Key fingerprint = 0FF7 E906 FBAF FE95 FD89 250D 52B1 AE95 6006 F40C

4

Hello,

I have synchronised with CVS and my patches for pg.in.dbf have not
applied cleanly. The present version in CVS can not compile.

You will find attached to this mail the plain versions (not patches)
which seems the best solution to put the CVS version up again.

There are also in CVS files .#* which have to be removed.

Note: when I made the patches I had no high bandwith connection so I
have retrieved the snapshot. Problems might have came from that.
I have no a high bandwith connection, so I will work with CVS directly
from now on.

Regards,
--
Thierry Laronde (Alceste) <tlaronde@polynum.org>
Key fingerprint = 0FF7 E906 FBAF FE95 FD89 250D 52B1 AE95 6006 F40C

5

Better with the file...

On Tue, Nov 04, 2003 at 11:55:31AM +0100, Thierry Laronde wrote:

Hello,

I have synchronised with CVS and my patches for pg.in.dbf have not
applied cleanly. The present version in CVS can not compile.

You will find attached to this mail the plain versions (not patches)
which seems the best solution to put the CVS version up again.

There are also in CVS files .#* which have to be removed.

Note: when I made the patches I had no high bandwith connection so I
have retrieved the snapshot. Problems might have came from that.
I have no a high bandwith connection, so I will work with CVS directly
from now on.

Regards,
--
Thierry Laronde (Alceste) <tlaronde@polynum.org>
Key fingerprint = 0FF7 E906 FBAF FE95 FD89 250D 52B1 AE95 6006 F40C

--
Thierry Laronde (Alceste) <tlaronde@polynum.org>
Key fingerprint = 0FF7 E906 FBAF FE95 FD89 250D 52B1 AE95 6006 F40C

(attachments)

pg.in.dbf.tar.gz (11.8 KB)

6

Sorry,

After looking at the CVS web interface to see the state of the files
on the server, the files are correct. The problem is on my side
the CVS updating being made via patches that didn't apply correctly
on my working copy.

So problem is mine and doesn't impact others.

Regards,
--
Thierry Laronde (Alceste) <tlaronde@polynum.org>
Key fingerprint = 0FF7 E906 FBAF FE95 FD89 250D 52B1 AE95 6006 F40C

7

Hi,

--- Thierry Laronde <tlaronde@polynum.com> wrote:

Hello,

I have synchronised with CVS and my patches for
pg.in.dbf have not
applied cleanly. The present version in CVS can not
compile.

It seems to me that the files in the tar.gz which
you've posted coincide with those committed by Paul.
They compiled OK with addition of missing pgdump.h
(I did add it).

Which version does not compile?

You will find attached to this mail the plain
versions (not patches)
which seems the best solution to put the CVS version
up again.

There are also in CVS files .#* which have to be
removed.

Again, where are these files and what are they?

There are the following in my
grass/src.garden/grass.postgresql/pg.in.dbf:

CVS
dbfopen.c
Gmakefile
local_proto.h
main.c
OBJ.i686-pc-linux-gnu
pgdump.c
pgdump.h
shapefil.h

Thierry Laronde (Alceste) <tlaronde@polynum.org>

Key fingerprint = 0FF7 E906 FBAF FE95 FD89 250D
52B1 AE95 6006 F40C

_______________________________________________
grass5 mailing list
grass5@grass.itc.it
http://grass.itc.it/mailman/listinfo/grass5

__________________________________
Do you Yahoo!?
Protect your identity with Yahoo! Mail AddressGuard
http://antispam.yahoo.com/whatsnewfree

8

Hello,

On Wed, Nov 05, 2003 at 02:08:37AM -0800, Alex Shevlakov wrote:

Hi,

--- Thierry Laronde <tlaronde@polynum.com> wrote:
> Hello,
>
> I have synchronised with CVS and my patches for
> pg.in.dbf have not
> applied cleanly. The present version in CVS can not
> compile.
>

It seems to me that the files in the tar.gz which
you've posted coincide with those committed by Paul.
They compiled OK with addition of missing pgdump.h
(I did add it).

Yes, I have verified yes too : the problem was on my side. The `cvs
update -dP' did not apply correctly on my client (for a reason I've not
tracked down yet) and since the 5.0 is to be released soon I emitted
the alert _before_ verifying that what I had was really what was on the
server.

Sorry for the noise.
--
Thierry Laronde (Alceste) <tlaronde@polynum.org>
Key fingerprint = 0FF7 E906 FBAF FE95 FD89 250D 52B1 AE95 6006 F40C

9

On Wed, 5 Nov 2003, Thierry Laronde wrote:

Hello,

On Wed, Nov 05, 2003 at 02:08:37AM -0800, Alex Shevlakov wrote:
> Hi,
>
> --- Thierry Laronde <tlaronde@polynum.com> wrote:
> > Hello,
> >
> > I have synchronised with CVS and my patches for
> > pg.in.dbf have not
> > applied cleanly. The present version in CVS can not
> > compile.
> >
>
> It seems to me that the files in the tar.gz which
> you've posted coincide with those committed by Paul.
> They compiled OK with addition of missing pgdump.h
> (I did add it).

Yes, I have verified yes too : the problem was on my side. The `cvs
update -dP' did not apply correctly on my client (for a reason I've not
tracked down yet) and since the 5.0 is to be released soon I emitted
the alert _before_ verifying that what I had was really what was on the
server.

I committed the changes to the (experimental) CVS HEAD so there was no
chance of them going into the released 5.0.3 version

Paul

10

On Wed, Nov 05, 2003 at 03:29:32PM +0000, Paul Kelly wrote:

On Wed, 5 Nov 2003, Thierry Laronde wrote:
>
> Yes, I have verified yes too : the problem was on my side. The `cvs
> update -dP' did not apply correctly on my client (for a reason I've not
> tracked down yet) and since the 5.0 is to be released soon I emitted
> the alert _before_ verifying that what I had was really what was on the
> server.

I committed the changes to the (experimental) CVS HEAD so there was no
chance of them going into the released 5.0.3 version

I thought that, implicitely, 5.0 was "feature frozen" hence that there
was only bug-fixes so in this case for me HEAD == TO_BE_RELEASED (at
least for src.garden since this is not to be continued in 5.7, right?).

What did I miss? And how the fixes can be tested if this is not
released? I was puzzled to see that in several years nobody hit the
bugs I've hit in my _first_ attempt to import a .DBF and concluded that
my chance has been to have to create first a GIS not having anything to
import from alien sources and that the ones who have tested GRASS trying
to import data have simply given up on the first attempt.

So may I say that these fixes are "hot fixes" and should be released
ASAP?

And unfortunately, the bugs I have fixed are different from the two ones
reported in the bug tracking system (or, to be more precise, I have made
a fix for these ones _too_ but this is only a partial fix and as you
have suspected, problems are not limited to pg.in.dbf but impact _all_
programs using the DBF library) [this was the theme of the message I've
sent this week-end about abstract data].
--
Thierry Laronde (Alceste) <tlaronde@polynum.org>
Key fingerprint = 0FF7 E906 FBAF FE95 FD89 250D 52B1 AE95 6006 F40C

11

Thierry Laronde wrote:

I was puzzled to see that in several years nobody hit the
bugs I've hit in my _first_ attempt to import a .DBF

I've found bugs in the libgis raster I/O code which date back to the
addition of NULL value support, and that is some of the most heavily
used code in GRASS.

A large part of the problem is the size of the GRASS code base (larger
than XFree86) relative to the number of users, meaning that there is a
lot of code which is hardly ever exercised.

--
Glynn Clements <glynn.clements@virgin.net>

12

[Stupid question: what is the policy for email message to the list?
Do people prefer to be emailed directly + Cc to the list for the
record, or is it OK to simply send to the list ---low traffic indeed]

On Thu, Nov 06, 2003 at 12:09:29PM +0000, Glynn Clements wrote:

A large part of the problem is the size of the GRASS code base (larger
than XFree86) relative to the number of users, meaning that there is a
lot of code which is hardly ever exercised.

IMHO the main priority is indeed to decrease the size of the code.

And this doesn't mean "loosing" features.

There are redundancies (DBMI versus src.garden for example).
There are programs using several copies of the (originally) same file,
copies which have diverged [one example that I know ;)]

./src.garden/grass.postgresql/pg.in.dbf/pgdump.c
./src.garden/grass.postgresql/v.in.arc.pg/pgdump.c
./src.garden/grass.postgresql/v.in.shape.pg/pgdump.c

=> this should be in a library

etc.

And perhaps applying a kind of Amdhal's law to identify the primitives
really used/ really needed.

But I don't claim saying anything new.
--
Thierry Laronde (Alceste) <tlaronde@polynum.org>
Key fingerprint = 0FF7 E906 FBAF FE95 FD89 250D 52B1 AE95 6006 F40C

13

> A large part of the problem is the size of the GRASS code base
> (larger than XFree86) relative to the number of users, meaning that
> there is a lot of code which is hardly ever exercised.

IMHO the main priority is indeed to decrease the size of the code.

Hopefully the natural place for this to be done is with the gradual code
migration to 5.7; vastly reorganizing the 5.3/HEAD branch would cause
many more headaches than it would solve, IMO. That along with (5.4)
being a dead-end in the code, I don't see the point in spending the time
and trouble on it during 5.3..

...

=> this should be in a library

I'd again plug for a stats library. This could be based on r.series or R
code, and GRASS modules could be rewritten to use it at the same time as
they are (properly) migrated to 5.7. I'm happy to fill in some functions
if someone more capable sets out the framework, although I think it
should really be written (or at least audited) by a real statistician,
as statistics is often a world of important subtleties that
non-statisticians (ie I) may overlook ..

just some thoughts,
Hamish