[GRASS-dev] [GRASS GIS] #2031: G_legal_filename() cleanup patch for GRASS 6

#2031: G_legal_filename() cleanup patch for GRASS 6
---------------------+------------------------------------------------------
Reporter: neteler | Owner: grass-dev@…
     Type: defect | Status: new
Priority: normal | Milestone: 6.4.4
Component: LibGIS | Version: svn-releasebranch64
Keywords: | Platform: All
      Cpu: All |
---------------------+------------------------------------------------------
Continued here from bug #2025:

G_legal_filename()...

Replying to [comment:9 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.

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/2031&gt;
GRASS GIS <http://grass.osgeo.org>

#2031: G_legal_filename() cleanup patch for GRASS 6
---------------------+------------------------------------------------------
Reporter: neteler | Owner: grass-dev@…
     Type: defect | Status: new
Priority: normal | Milestone: 6.4.4
Component: LibGIS | Version: svn-releasebranch64
Keywords: | Platform: All
      Cpu: All |
---------------------+------------------------------------------------------

Comment(by neteler):

The attached patch additionally requires:

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

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/2031#comment:1&gt;
GRASS GIS <http://grass.osgeo.org>

#2031: G_legal_filename() cleanup patch for GRASS 6
----------------------+-----------------------------------------------------
  Reporter: neteler | Owner: grass-dev@…
      Type: defect | Status: closed
  Priority: normal | Milestone: 6.4.4
Component: LibGIS | Version: svn-releasebranch64
Resolution: fixed | Keywords:
  Platform: All | Cpu: All
----------------------+-----------------------------------------------------
Changes (by neteler):

  * status: new => closed
  * resolution: => fixed

Comment:

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

Closing.

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/2031#comment:2&gt;
GRASS GIS <http://grass.osgeo.org>

#2031: G_legal_filename() cleanup patch for GRASS 6
----------------------+-----------------------------------------------------
  Reporter: neteler | Owner: grass-dev@…
      Type: defect | Status: closed
  Priority: normal | Milestone: 6.4.4
Component: LibGIS | Version: svn-releasebranch64
Resolution: fixed | Keywords:
  Platform: All | Cpu: All
----------------------+-----------------------------------------------------

Comment(by neteler):

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

Closing.

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/2031#comment:3&gt;
GRASS GIS <http://grass.osgeo.org>

#2031: G_legal_filename() cleanup patch for GRASS 6
----------------------+-----------------------------------------------------
  Reporter: neteler | Owner: grass-dev@…
      Type: defect | Status: closed
  Priority: normal | Milestone: 6.4.4
Component: LibGIS | Version: svn-releasebranch64
Resolution: fixed | Keywords:
  Platform: All | Cpu: All
----------------------+-----------------------------------------------------

Comment(by 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

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/2031#comment:4&gt;
GRASS GIS <http://grass.osgeo.org>

#2031: G_legal_filename() cleanup patch for GRASS 6
----------------------+-----------------------------------------------------
  Reporter: neteler | Owner: grass-dev@…
      Type: defect | Status: closed
  Priority: normal | Milestone: 6.4.4
Component: LibGIS | Version: svn-releasebranch64
Resolution: fixed | Keywords:
  Platform: All | Cpu: All
----------------------+-----------------------------------------------------

Comment(by neteler):

Replying to [comment:4 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

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/2031#comment:5&gt;
GRASS GIS <http://grass.osgeo.org>

#2031: G_legal_filename() cleanup patch for GRASS 6
----------------------+-----------------------------------------------------
  Reporter: neteler | Owner: grass-dev@…
      Type: defect | Status: closed
  Priority: normal | Milestone: 6.4.4
Component: LibGIS | Version: svn-releasebranch64
Resolution: fixed | Keywords:
  Platform: All | Cpu: All
----------------------+-----------------------------------------------------

Comment(by neteler):

Replying to [comment:4 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.

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/2031#comment:6&gt;
GRASS GIS <http://grass.osgeo.org>

#2031: G_legal_filename() cleanup patch for GRASS 6
----------------------+-----------------------------------------------------
  Reporter: neteler | Owner: grass-dev@…
      Type: defect | Status: closed
  Priority: normal | Milestone: 6.4.4
Component: LibGIS | Version: svn-releasebranch64
Resolution: fixed | Keywords:
  Platform: All | Cpu: All
----------------------+-----------------------------------------------------

Comment(by glynn):

Replying to [comment:6 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).

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/2031#comment:7&gt;
GRASS GIS <http://grass.osgeo.org>

#2031: G_legal_filename() cleanup patch for GRASS 6
----------------------+-----------------------------------------------------
  Reporter: neteler | Owner: grass-dev@…
      Type: defect | Status: closed
  Priority: normal | Milestone: 6.4.4
Component: LibGIS | Version: svn-releasebranch64
Resolution: fixed | Keywords:
  Platform: All | Cpu: All
----------------------+-----------------------------------------------------

Comment(by martinl):

Replying to [comment:7 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?

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/2031#comment:8&gt;
GRASS GIS <http://grass.osgeo.org>

#2031: G_legal_filename() cleanup patch for GRASS 6
----------------------+-----------------------------------------------------
  Reporter: neteler | Owner: grass-dev@…
      Type: defect | Status: closed
  Priority: normal | Milestone: 6.4.4
Component: LibGIS | Version: svn-releasebranch64
Resolution: fixed | Keywords:
  Platform: All | Cpu: All
----------------------+-----------------------------------------------------

Comment(by glynn):

Replying to [comment:8 martinl]:

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

Not that I know of.

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/2031#comment:9&gt;
GRASS GIS <http://grass.osgeo.org>

#2031: G_legal_filename() cleanup patch for GRASS 6
----------------------+-----------------------------------------------------
  Reporter: neteler | Owner: grass-dev@…
      Type: defect | Status: closed
  Priority: normal | Milestone: 6.4.4
Component: LibGIS | Version: svn-releasebranch64
Resolution: fixed | Keywords:
  Platform: All | Cpu: All
----------------------+-----------------------------------------------------

Comment(by 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

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/2031#comment:10&gt;
GRASS GIS <http://grass.osgeo.org>

On 19/05/14 05:30, GRASS GIS wrote:

  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

How about my proposal from a few weeks ago: Nobody commits to grass64release, only to grass6dev, and Hamish is the official maintainer of grass64release and decides what can be backported ?

This obviously can only work if Hamish has the time, so that 6.4.4 is not stalled for lack of manpower.

Moritz

Hi,

2014-05-19 9:50 GMT+02:00 Moritz Lennert <mlennert@club.worldonline.be>:

[...]

How about my proposal from a few weeks ago: Nobody commits to
grass64release, only to grass6dev, and Hamish is the official maintainer of
grass64release and decides what can be backported ?

This obviously can only work if Hamish has the time, so that 6.4.4 is not
stalled for lack of manpower.

from my personal experience I would say that than we will probably
never release any new GRASS 6 version. :wink:

Martin

--
Martin Landa * http://geo.fsv.cvut.cz/gwiki/Landa

Mortiz wrote:

How about my proposal from a few weeks ago: Nobody commits to
grass64release, only to grass6dev, and Hamish is the official
maintainer of grass64release and decides what can be backported ?

This obviously can only work if Hamish has the time, so that 6.4.4
is not stalled for lack of manpower.

Well, I don't like being the sole gatekeeper, both for community and
logistical reasons. I am happy and pleased for everyone to backport, as
long as we can all be working from the same ideas about where we are in
a freeze and what our expectations for it are. Which is primarily (and
luckily) a solvable communication problem, and not a structural,
technical, or personality one.

Having said that, if people find backporting to 6 is too much of a pain,
simply leave a ticket open with the next release as the target version
and a request to backport and I'll try to take of it for them, in
$unknown time.

Through bitter experience and brown bags I'm now really strict with
myself about committing anything, no matter how innocent looking, to the
release branch. What has been driving me nuts is spending all these
hours carefully testing and trying to spend a lot of energy on attention
to detail, and to then observe other developers just come through with a
bulldozer and commit stuff haphazard all over the garden I'd just spent
so many hours cleaning up. If all that time ends up in a hole, why
bother?

I note a revert needed in devbr6 where a number of features-in-testing
(64 bit support, stat() checks in libgis, ...) recently got blown away
by a mass backwards-sync with relbr64. I can't tell you how demotivating
it is to have all those hours of work ignored and removed. Now I have to
spend even more hours putting them back by hand, because I doubt any one
else would care enough about devbr6 to fix it. This is not fun, and if
it is not fun I have to remind myself what I'm doing here. So in the
last weeks instead of working or even thinking about that stuff I have
concentrated on what I really do enjoy, the creative outlet aspect of
coding. Having to be the guy who says "no" all the time really eats into
my enjoyment of working in a team, I hate it, it's not a healthy way to
be.

For sure I play it a bit too conservatively some times, and unadvertised
devil's advocate others, and it is noted that this slows down releases,
which frustrates and drops motivation in others too. But I'm ok with not
having to be right all the time about where the right balance is since I
can relax knowing everyone is doing what they consider to be best and
positive, and they are really smart and often right about it. And it is
frustrating to me too for the releases to take so long, especially since
I know some of it is me saying "no" but not having the extra time to do
the review and edits to help solve the issues, many of which are of my
own making.

It's a classic question I don't have an answer to: we can use the time
just before a release as a huge community motivator to get all the last
minute bugs fixed and all the last minute things people wanted to see in
before the next release. But at the same time it's the absolute worst
time to make changes which can't have time for testing. So how to
harness all that energy without breaking anything by accident?

My personal strategy has been to divert non-critical things in to
release+1, then soon after release go through the devbr and backport it
all. Yes it slips a release which may be many months apart, but I treat
it like AGU meetings: you can't be everywhere and do everything at once,
so it's ok, do what you can and enjoy it guilt free.

Then there are things like the parallelized shell scripts in devbr6
which are wonderful and seem to be working fine, but I've not backported
them because I don't know the answer to allow SIGINT to also kill the
subprocesses. From the command line with `top` and maybe gkrellm running
as a cpu/process monitor it's pretty obvious they are still going. But
from my testing of the msys/bash background& a series of jobs + `wait`
strategy with the wxGUI on MS Windows (it works!) that if you forget to
change the computational region away from 1 meter, as is easy to do
there, the r.univar job is going to take hours. So after a while on 1%
done you press the "stop" button in the module's gui window but the
children keep going in the background. Probably most Windows users
wouldn't notice that, only that the machine got slow. So I don't have
an answer to that problem and thus the new code remains waiting in
devbr6. (I'd note parallelized python scripts in grass7, which by
structural necessity of python only support multi-processes not
multi-thread, have the same exact problem..)

Martin:

from my personal experience I would say that than we will probably
never release any new GRASS 6 version. :wink:

fwiw I felt that going into the recent code sprint we were ready for
6.4.4rc1, r.li was the last thing to solve. And now we're not. If we're
ever going to get there though we have to start by agreeing on a freeze
strategy for it. So AFAIU we are in pre-release hard freeze for
relbr64: significant bug fixes and spelling mistakes only. And devbr6
is effectively done for refactoring and significant new development.
But I wouldn't be absolutist about it, any engineer will tell you there
is great strength in flexibility. If there is some really nice feature
or new flag for better compatibility or great speedup from grass7 in a
leaf module I wouldn't mind it eventually drifting down into stable
release+1. (recent horizontal legends for ps.map come to mind, although
that one might be argued as a bugfix :slight_smile:

thanks for listening and caring about grass too,
Hamish

On Mon, May 19, 2014 at 2:25 PM, Hamish <hamish.webmail@gmail.com> wrote:

Well, I don't like being the sole gatekeeper,

But your wording is exactly like this.

Having said that, if people find backporting to 6 is too much of a pain,

I guess I am among those who have done most of the backports in the
past 10-15 years.
It is a pain and it needs to be done. And so we did.

I note a revert needed in devbr6 where a number of features-in-testing
(64 bit support, stat() checks in libgis, ...) recently got blown away
by a mass backwards-sync with relbr64.

Do you refer to r.li? It was completely broken. Adding parallel
support to completely broken tools does not help.

For sure I play it a bit too conservatively some times, and unadvertised
devil's advocate others, and it is noted that this slows down releases,
which frustrates and drops motivation in others too.

Yes. It especially drives away our users.

fwiw I felt that going into the recent code sprint we were ready for
6.4.4rc1, r.li was the last thing to solve. And now we're not.

Well, a VERY few people contributed in the past weeks to 6.4. Please
be fair to attribute the "we" to those who have been investing hours
of their time. Just see the ChangeLog for names.

Markus

#2031: G_legal_filename() cleanup patch for GRASS 6
----------------------+-----------------------------------------------------
  Reporter: neteler | Owner: grass-dev@…
      Type: defect | Status: closed
  Priority: normal | Milestone: 6.4.4
Component: LibGIS | Version: svn-releasebranch64
Resolution: fixed | Keywords:
  Platform: All | Cpu: All
----------------------+-----------------------------------------------------

Comment(by neteler):

Replying to [comment:10 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.

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/2031#comment:11&gt;
GRASS GIS <http://grass.osgeo.org>

#2031: G_legal_filename() cleanup patch for GRASS 6
----------------------+-----------------------------------------------------
  Reporter: neteler | Owner: grass-dev@…
      Type: defect | Status: closed
  Priority: normal | Milestone: 6.4.4
Component: LibGIS | Version: svn-releasebranch64
Resolution: fixed | Keywords:
  Platform: All | Cpu: All
----------------------+-----------------------------------------------------

Comment(by mlennert):

Replying to [comment:11 neteler]:
> Replying to [comment:10 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 ?

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/2031#comment:12&gt;
GRASS GIS <http://grass.osgeo.org>

#2031: G_legal_filename() cleanup patch for GRASS 6
----------------------+-----------------------------------------------------
  Reporter: neteler | Owner: grass-dev@…
      Type: defect | Status: closed
  Priority: normal | Milestone: 6.4.4
Component: LibGIS | Version: svn-releasebranch64
Resolution: fixed | Keywords:
  Platform: All | Cpu: All
----------------------+-----------------------------------------------------

Comment(by glynn):

Replying to [comment:12 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@mapset.suffix), 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.

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/2031#comment:13&gt;
GRASS GIS <http://grass.osgeo.org>

#2031: G_legal_filename() cleanup patch for GRASS 6
----------------------+-----------------------------------------------------
  Reporter: neteler | Owner: grass-dev@…
      Type: defect | Status: closed
  Priority: normal | Milestone: 6.4.4
Component: LibGIS | Version: svn-releasebranch64
Resolution: fixed | Keywords:
  Platform: All | Cpu: All
----------------------+-----------------------------------------------------

Comment(by neteler):

Replying to [comment:13 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.

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/2031#comment:14&gt;
GRASS GIS <http://grass.osgeo.org>