G_legal_filename() cleanup patch for GRASS 6

[neteler]

Continued here from bug #2025:

G_legal_filename()...

Replying to glynn:

... is meant for anything which will be a single component of a pathname: either an unqualified map name, or a mapset name, or a location name.

Any characters which have "meaning" to GRASS or to the OS are not "legal" in this sense, so it rejects names which contain e.g. "@" (mapset separator) or "/" (directory separator).

Attached a backport of r32820 from GRASS 7. Please verify.

[neteler]

Backport of r32820 to relbranch64

[neteler]

The attached patch additionally requires:

svn rm raster/r.surf.fractal/interface.c

[neteler]

Backport of r32820 committed to relbr64 in r and to devbr6 in r

Closing.

[neteler]

Backport of r32820 committed to relbr64 in r60000 and to devbr6 in r60001

Closing.

[hamish]

Would it not have been better to replace G_legal_filename() with some G_legal_mapname() than to just remove all the safety checks?

cell/mapname is by definition a filesystem name, so G_legal_filename() seems perfectly legitimate safety check.

see ​https://trac.osgeo.org/grass/ticket/2025#comment:9

and comment 8, just above it: "OK no problem, proposed backport of r32820 deleted."

This seems more like refactoring than specific bug fixing to me, so not really appropriate for 6.x anyway, and certainly not something to do just before releasing 6.4.4. Have the appropriate checks been added in libgis for all functions which will fail?

Also there is refactoring of v.random.surface and r.surf.fractal, was that intended to be in this commit?

I suggest to revert -- and not commit large patches to relbr64 directly! Especially when we are trying to get a release out the door!!

thanks, Hamish

[neteler]

Replying to hamish:

cell/mapname is by definition a filesystem name, so G_legal_filename() seems perfectly legitimate safety check.

See the ML discussion for this.

Also there is refactoring of v.random.surface and

You mean r.random.surface? That's part of the change r32820.

r.surf.fractal, was that intended to be in this commit?

That's a trivial change and part of the original change.

I suggest to revert -- and not commit large patches to relbr64 directly! Especially when we are trying to get a release out the door!!

I oppose to revert and the change r32820 has been tested for 6 years in trunk. There is no release soon, we do not even have an RC1 for 6.4 as no feedback there.

thanks, Markus

[neteler]

Replying to hamish:

Would it not have been better to replace G_legal_filename() with some G_legal_mapname() than to just remove all the safety checks?

IMHO rather than having this test in hundreds of modules it should be done in the respective library function.

[glynn]

Replying to neteler:

IMHO rather than having this test in hundreds of modules it should be done in the respective library function.

Ideally, G_parser() would perform the checks. But that would require either a significant amount of fairly ugly code, or redesigning the option descriptions (which has been on the table since roughly forever, but with no actual progress).

[martinl]

Replying to glynn:

Ideally, G_parser() would perform the checks. But that would require either a significant amount of fairly ugly code, or redesigning the option descriptions (which has been on the table since roughly forever, but with no actual progress).

Is there any wiki (ideally on trac) page which collects ideas how to redesign the option descriptions?

[glynn]

Replying to martinl:

Is there any wiki (ideally on trac) page which collects ideas how to redesign the option descriptions?

Not that I know of.

[hamish]

Hi,

r60000,1 need to be reverted. There are tons of refactoring changes in trunk which do not belong in G6. This is not a bug fix, just removal of many safety checks.

This is a blocker for 6.4.4 AFAIAC.

MarkusN:

IMHO rather than having this test in hundreds of modules it should be done in the respective library function.

Sure, start by doing it in the library too, and in the parser, as multiple layers of safety. I don't object to that at all, but for G6 it is simple: do not risk changing stuff which does not need to be changed. The law of unintended consequences is just too strong. Refactoring goes in trunk not the stable branch.

It was already established in #2025, commants 7 & 8 that this should not be backported.

The bug in question (AIUI) was limiting output non-map data (text, image, ..) files written out to the filesystem by artificial legal_filename tests on them, so if you wanted output="summary.txt" to be output="C:\Users\me\summary.txt" you were blocked. What r60000 seems to do is remove the legal_filename checks for grass raster/vector map map names which should keep the checks. And I'd argue that as long as the library checks are missing in G6's parser and scattered library functions (as in many cases they seem to be), it is better to have the module fail at startup with the sometimes-gratuitous checks rather than waiting to see it fail at the end when it tries to write out the result map, probably leaving (possibly large) .tmp/ files behind too.

And again, even if this were a valid candidate for G6, backporting stuff directly to 6.4 without going into devbr6 for at least light/any testing first, especially anything which touches core libgis, especially a time right before RC release, is incredibly risky for anything which is not a critical bug fix, and not an environment I can justify contributing my time to.

sorry if I'm grumpy, but I'm at the end of my rope about this.

regards, Hamish

[neteler]

Replying to hamish:

r60000,1 need to be reverted.

Feel free to revert. But please explain shortly why

r32820 "G_legal_filename() is for files, not maps"

should not be reverted, too.

[mlennert]

Replying to neteler:

Replying to hamish:

r60000,1 need to be reverted.

Feel free to revert. But please explain shortly why

r32820 "G_legal_filename() is for files, not maps"

should not be reverted, too.

Legitimate question: it seems to me that r32820 mixes cases where the output are (non-GRASS element) files (e.g. r.kappa) with cases where the output are GRASS element files (e.g. r.buffer, r.median, r.neighbors). Why was the change necessary/advisable for the latter ?

[glynn]

Replying to mlennert:

Legitimate question: it seems to me that r32820 mixes cases where the output are (non-GRASS element) files (e.g. r.kappa) with cases where the output are GRASS element files (e.g. r.buffer, r.median, r.neighbors). Why was the change necessary/advisable for the latter ?

G__open_raster_new() accepts a fully-qualified name for an output map, so long as the mapset matches the current mapset. G__open_raster_new() performs this check, removes any @mapset part, then performs the G_legal_filename() check on the unqualified name.

So a G_legal_filename() call in the module is typically redundant (the open_new function will reject invalid names soon enough), and will reject qualified names which may otherwise work fine. I say "may" because I don't know whether the functions for writing support files (history, colour tables, etc) also cope with qualified names.

If the G_legal_filename() calls are required to protect functions other than G_open_*_new(), they should remain. Otherwise, they should be removed.

In 7.x, the library functions are quite consistent about accepting qualified names. Modules should normally be able to pass names from option values straight through to library functions without any checks or parsing. The only cases which are problematic are where an option specifies a base name to which a suffix is appended (i.e. you need to generate map.suffix@mapset, while simply appending the suffix will produce map@…), modules which explicitly deal with mapsets or locations (e.g. r.proj), or database-management utilities (e.g. g.mlist).

Having said that, it would have been better if r32820 had been split into separate patches, one to remove bogus checks (e.g. names of non-GRASS data files) and another to remove redundant checks.

[neteler]

Replying to glynn:

If the G_legal_filename() calls are required to protect functions other than G_open_*_new(), they should remain. Otherwise, they should be removed.

For now all G_legal_filename() calls reinstated in relbr64: r60401; develbr6: r60402.