setup and colons in filenames

setup and colons in filenames

[Ken Brown]

This is a followup to the thread started here:

   https://cygwin.com/ml/cygwin-patches/2017-q4/msg00012.html

Currently setup's parse_filename is not correctly parsing filenames in 
/etc/setup/installed.db that contain colons, as explained in the above 
thread.  It would be easy to fix this by just ripping out the 'base' 
function, except for the fact that parse_filename is called by 
ScanFindVisitor::visitFile.

I don't know enough about WIN32_FIND_DATA to know whether the call to 
'base' is needed for that use of parse_filename.  If so, is it safe to 
skip all colons in that setting, since we're dealing with Win32 
filenames and they don't see the colons in Cygwin filenames?

Do we need two versions of parse_filename, one that calls base and one 
that doesn't?

Ken

Re: setup and colons in filenames

[Jon Turney]

On 25/10/2017 16:50, Ken Brown wrote:
> This is a followup to the thread started here:
> 
>    https://cygwin.com/ml/cygwin-patches/2017-q4/msg00012.html
> 
> Currently setup's parse_filename is not correctly parsing filenames in 
> /etc/setup/installed.db that contain colons, as explained in the above 
> thread.  It would be easy to fix this by just ripping out the 'base' 
> function, except for the fact that parse_filename is called by 
> ScanFindVisitor::visitFile.

Since older setup cannot correctly parse an installed.db containing 
filenames like that, we should probably bump the installed.db version at 
the same time as fixing this.

> I don't know enough about WIN32_FIND_DATA to know whether the call to 
> 'base' is needed for that use of parse_filename.  If so, is it safe to 
> skip all colons in that setting, since we're dealing with Win32 
> filenames and they don't see the colons in Cygwin filenames?

Yeah, that's about as far as I got before giving up...

> Do we need two versions of parse_filename, one that calls base and one 
> that doesn't?

This might be the easiest solution :)

The other concern I had was if the filenames for the package archives 
stored in the download cache end up containing a ':', which I thought 
wasn't allowed in windows filenames?

Re: setup and colons in filenames

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 2293 bytes --]

On Oct 25 20:23, Jon Turney wrote:
> On 25/10/2017 16:50, Ken Brown wrote:
> > This is a followup to the thread started here:
> > 
> >    https://cygwin.com/ml/cygwin-patches/2017-q4/msg00012.html
> > 
> > Currently setup's parse_filename is not correctly parsing filenames in
> > /etc/setup/installed.db that contain colons, as explained in the above
> > thread.  It would be easy to fix this by just ripping out the 'base'
> > function, except for the fact that parse_filename is called by
> > ScanFindVisitor::visitFile.
> 
> Since older setup cannot correctly parse an installed.db containing
> filenames like that, we should probably bump the installed.db version at the
> same time as fixing this.
> 
> > I don't know enough about WIN32_FIND_DATA to know whether the call to
> > 'base' is needed for that use of parse_filename.  If so, is it safe to
> > skip all colons in that setting, since we're dealing with Win32
> > filenames and they don't see the colons in Cygwin filenames?
> 
> Yeah, that's about as far as I got before giving up...
> 
> > Do we need two versions of parse_filename, one that calls base and one
> > that doesn't?
> 
> This might be the easiest solution :)
> 
> The other concern I had was if the filenames for the package archives stored
> in the download cache end up containing a ':', which I thought wasn't
> allowed in windows filenames?

Colons in Cygwin filenames will have 0xf03a value in WIN32.  The code to
transpose special chars into the private use area at 0xf0XY is in setup,
but I'm not sure if setup is really working correctly with archives
containing a colon.  That is:

- downloading the archive to the local drive with the filename
  transposed (rather than failing to store the file)
- storing the filename with a real colon in installed.db and still
  being able to transpose the filename when fetched from installed.db.

Especially the latter part is interesting since the reverse operation
(transposing back from 0xf0XY to 0xXY) may not be implemented in setup,
yet.  If so, I don't remember.  Somebody will have to test this.


Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: setup and colons in filenames

[Jon Turney]

On 25/10/2017 21:16, Corinna Vinschen wrote:
> On Oct 25 20:23, Jon Turney wrote:
>> On 25/10/2017 16:50, Ken Brown wrote:
>>> This is a followup to the thread started here:
>>>
>>>     https://cygwin.com/ml/cygwin-patches/2017-q4/msg00012.html
>>>
>>> Currently setup's parse_filename is not correctly parsing filenames in
>>> /etc/setup/installed.db that contain colons, as explained in the above
>>> thread.  It would be easy to fix this by just ripping out the 'base'
>>> function, except for the fact that parse_filename is called by
>>> ScanFindVisitor::visitFile.
>>
>> Since older setup cannot correctly parse an installed.db containing
>> filenames like that, we should probably bump the installed.db version at the
>> same time as fixing this.
>>
>>> I don't know enough about WIN32_FIND_DATA to know whether the call to
>>> 'base' is needed for that use of parse_filename.  If so, is it safe to
>>> skip all colons in that setting, since we're dealing with Win32
>>> filenames and they don't see the colons in Cygwin filenames?
>>
>> Yeah, that's about as far as I got before giving up...
>>
>>> Do we need two versions of parse_filename, one that calls base and one
>>> that doesn't?
>>
>> This might be the easiest solution :)
>>
>> The other concern I had was if the filenames for the package archives stored
>> in the download cache end up containing a ':', which I thought wasn't
>> allowed in windows filenames?
> 
> Colons in Cygwin filenames will have 0xf03a value in WIN32.  The code to
> transpose special chars into the private use area at 0xf0XY is in setup,
> but I'm not sure if setup is really working correctly with archives
> containing a colon.
I think the package archives are stored using native Windows filenames, 
not cygwin filenames (i.e. file:// paths rather than cygfile:// paths), 
because the "Local Package Directory" is not necessarily under the 
cygwin root, so I'm not sure that transformation applies.

Re: setup and colons in filenames

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 2519 bytes --]

On Oct 25 21:36, Jon Turney wrote:
> On 25/10/2017 21:16, Corinna Vinschen wrote:
> > On Oct 25 20:23, Jon Turney wrote:
> > > On 25/10/2017 16:50, Ken Brown wrote:
> > > > This is a followup to the thread started here:
> > > > 
> > > >     https://cygwin.com/ml/cygwin-patches/2017-q4/msg00012.html
> > > > 
> > > > Currently setup's parse_filename is not correctly parsing filenames in
> > > > /etc/setup/installed.db that contain colons, as explained in the above
> > > > thread.  It would be easy to fix this by just ripping out the 'base'
> > > > function, except for the fact that parse_filename is called by
> > > > ScanFindVisitor::visitFile.
> > > 
> > > Since older setup cannot correctly parse an installed.db containing
> > > filenames like that, we should probably bump the installed.db version at the
> > > same time as fixing this.
> > > 
> > > > I don't know enough about WIN32_FIND_DATA to know whether the call to
> > > > 'base' is needed for that use of parse_filename.  If so, is it safe to
> > > > skip all colons in that setting, since we're dealing with Win32
> > > > filenames and they don't see the colons in Cygwin filenames?
> > > 
> > > Yeah, that's about as far as I got before giving up...
> > > 
> > > > Do we need two versions of parse_filename, one that calls base and one
> > > > that doesn't?
> > > 
> > > This might be the easiest solution :)
> > > 
> > > The other concern I had was if the filenames for the package archives stored
> > > in the download cache end up containing a ':', which I thought wasn't
> > > allowed in windows filenames?
> > 
> > Colons in Cygwin filenames will have 0xf03a value in WIN32.  The code to
> > transpose special chars into the private use area at 0xf0XY is in setup,
> > but I'm not sure if setup is really working correctly with archives
> > containing a colon.
> I think the package archives are stored using native Windows filenames, not
> cygwin filenames (i.e. file:// paths rather than cygfile:// paths), because
> the "Local Package Directory" is not necessarily under the cygwin root, so
> I'm not sure that transformation applies.

The transposition is handled in mklongpath, which is called in nt_fopen,
or called manually in other callers of nt_wfopen to construct the long
path.  In theory, this should cover all bases...


Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: setup and colons in filenames

[Jon Turney]

On 25/10/2017 22:00, Corinna Vinschen wrote:
> On Oct 25 21:36, Jon Turney wrote:
>> On 25/10/2017 21:16, Corinna Vinschen wrote:
>>> On Oct 25 20:23, Jon Turney wrote:
>>>> On 25/10/2017 16:50, Ken Brown wrote:
>>>>> This is a followup to the thread started here:
>>>>
>>>> The other concern I had was if the filenames for the package archives stored
>>>> in the download cache end up containing a ':', which I thought wasn't
>>>> allowed in windows filenames?
>>>
>>> Colons in Cygwin filenames will have 0xf03a value in WIN32.  The code to
>>> transpose special chars into the private use area at 0xf0XY is in setup,
>>> but I'm not sure if setup is really working correctly with archives
>>> containing a colon.
>> I think the package archives are stored using native Windows filenames, not
>> cygwin filenames (i.e. file:// paths rather than cygfile:// paths), because
>> the "Local Package Directory" is not necessarily under the cygwin root, so
>> I'm not sure that transformation applies.
> 
> The transposition is handled in mklongpath, which is called in nt_fopen,
> or called manually in other callers of nt_wfopen to construct the long
> path.  In theory, this should cover all bases...

Yes, you are quite correct, and this transformation is happening, so 
there is no problem here.

(I think what I did to confuse myself was look at the "Local Package 
Directory" in Cygwin shell without even thinking about it, and saw the 
filenames being shown with a ':', while thinking they were Windows 
filenames...)

Re: setup and colons in filenames

[Jon Turney]

On 25/10/2017 20:23, Jon Turney wrote:
> On 25/10/2017 16:50, Ken Brown wrote:
>> This is a followup to the thread started here:
>>
>>    https://cygwin.com/ml/cygwin-patches/2017-q4/msg00012.html
>>
>> Currently setup's parse_filename is not correctly parsing filenames in 
>> /etc/setup/installed.db that contain colons, as explained in the above 
>> thread.  It would be easy to fix this by just ripping out the 'base' 
>> function, except for the fact that parse_filename is called by 
>> ScanFindVisitor::visitFile.
> 
> Since older setup cannot correctly parse an installed.db containing 
> filenames like that, we should probably bump the installed.db version at 
> the same time as fixing this.
> 
>> I don't know enough about WIN32_FIND_DATA to know whether the call to 
>> 'base' is needed for that use of parse_filename.  If so, is it safe to 
>> skip all colons in that setting, since we're dealing with Win32 
>> filenames and they don't see the colons in Cygwin filenames?
> 
> Yeah, that's about as far as I got before giving up...
> 
>> Do we need two versions of parse_filename, one that calls base and one 
>> that doesn't?
> 
> This might be the easiest solution :)

It gets better:  We only go down the whole route of using 
ScanFindVisitor::visitFile() from do_from_local_dir() if we couldn't 
find an .ini file.

If there's no .ini file found, we look at every package archive and
parse the version out of the archive filename.

This seems a really odd thing to do, as we've no idea about the 
dependencies of these packages, so installing them is unlikely work well.

Removing all the setup.ini files from my package directory, the good 
news is that WIN32_FIND_DATA just returns the filename, not a full 
pathname, so this use of base() isn't needed, either.

The bad news is that setup then exits (this works ok in 2.882, so 
something's got broken somewhere...)

I'm kind of tempted to remove this mis-feature.

Re: setup and colons in filenames

[Ken Brown]

On 10/26/2017 12:14 PM, Jon Turney wrote:
> On 25/10/2017 20:23, Jon Turney wrote:
>> On 25/10/2017 16:50, Ken Brown wrote:
>>> This is a followup to the thread started here:
>>>
>>>    https://cygwin.com/ml/cygwin-patches/2017-q4/msg00012.html
>>>
>>> Currently setup's parse_filename is not correctly parsing filenames 
>>> in /etc/setup/installed.db that contain colons, as explained in the 
>>> above thread.  It would be easy to fix this by just ripping out the 
>>> 'base' function, except for the fact that parse_filename is called by 
>>> ScanFindVisitor::visitFile.
>>
>> Since older setup cannot correctly parse an installed.db containing 
>> filenames like that, we should probably bump the installed.db version 
>> at the same time as fixing this.
>>
>>> I don't know enough about WIN32_FIND_DATA to know whether the call to 
>>> 'base' is needed for that use of parse_filename.  If so, is it safe 
>>> to skip all colons in that setting, since we're dealing with Win32 
>>> filenames and they don't see the colons in Cygwin filenames?
>>
>> Yeah, that's about as far as I got before giving up...
>>
>>> Do we need two versions of parse_filename, one that calls base and 
>>> one that doesn't?
>>
>> This might be the easiest solution :)
> 
> It gets better:  We only go down the whole route of using 
> ScanFindVisitor::visitFile() from do_from_local_dir() if we couldn't 
> find an .ini file.
> 
> If there's no .ini file found, we look at every package archive and
> parse the version out of the archive filename.
> 
> This seems a really odd thing to do, as we've no idea about the 
> dependencies of these packages, so installing them is unlikely work well.
> 
> Removing all the setup.ini files from my package directory, the good 
> news is that WIN32_FIND_DATA just returns the filename, not a full 
> pathname, so this use of base() isn't needed, either.
> 
> The bad news is that setup then exits (this works ok in 2.882, so 
> something's got broken somewhere...)

What happens is that ScanFindVisitor::visitFile doesn't set various 
attributes of the packages it creates (such as SDesc).  This leads to a 
crash when the SolvableVersion methods (such as SDesc()) return null 
pointers that get dereferenced by PickView.

> I'm kind of tempted to remove this mis-feature.

I agree strongly.  I was about to write an email suggesting the same 
thing when I saw your mail.  Unless you're already working on it, I'll 
be glad to prepare a patch series that does this and also takes care of 
whatever is needed for supporting colons in installed.db

Ken

Re: setup and colons in filenames

[Jon Turney]

On 26/10/2017 18:04, Ken Brown wrote:
> On 10/26/2017 12:14 PM, Jon Turney wrote:
> 
>> I'm kind of tempted to remove this mis-feature.
> 
> I agree strongly.  I was about to write an email suggesting the same 
> thing when I saw your mail.  Unless you're already working on it, I'll 
> be glad to prepare a patch series that does this and also takes care of 
> whatever is needed for supporting colons in installed.db

Please do so.

Re: setup and colons in filenames

[Achim Gratz]

Jon Turney writes:
> This seems a really odd thing to do, as we've no idea about the
> dependencies of these packages, so installing them is unlikely work
> well.

This particular code path was vetoed from getting thrown out last time I
worked in that area since it would break long-standing expectations
w.r.t. local package archoves that have no setup.ini or even *.hint.

> I'm kind of tempted to remove this mis-feature.

In a properly maintained mirror or local archive it is a mis-feature.  I
was told not everyone wants to have to do the extra steps of doing that
maintenance.  At least if we detect what should be a proper package repo
we don't additionally look at all the files anymore.


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Factory and User Sound Singles for Waldorf Blofeld:
http://Synth.Stromeko.net/Downloads.html#WaldorfSounds

Re: setup and colons in filenames

[Jon Turney]

On 26/10/2017 18:42, Achim Gratz wrote:
> Jon Turney writes:
>> This seems a really odd thing to do, as we've no idea about the
>> dependencies of these packages, so installing them is unlikely work
>> well.
> 
> This particular code path was vetoed from getting thrown out last time I
> worked in that area since it would break long-standing expectations
> w.r.t. local package archoves that have no setup.ini or even *.hint.

Can we have a link to that discussion, please?

>> I'm kind of tempted to remove this mis-feature.
> 
> In a properly maintained mirror or local archive it is a mis-feature.  I
> was told not everyone wants to have to do the extra steps of doing that
> maintenance.  At least if we detect what should be a proper package repo
> we don't additionally look at all the files anymore.

Re: setup and colons in filenames

[Achim Gratz]

Jon Turney writes:
> Can we have a link to that discussion, please?

It's somewhere in that thread from June 2015 related to the changes
between 2.871 and 2.873.  Also one of the intermediate versions got
complaints due to that code path not working correctly.


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Samples for the Waldorf Blofeld:
http://Synth.Stromeko.net/Downloads.html#BlofeldSamplesExtra

Re: setup and colons in filenames

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 909 bytes --]

On Oct 26 20:25, Achim Gratz wrote:
> Jon Turney writes:
> > Can we have a link to that discussion, please?
> 
> It's somewhere in that thread from June 2015 related to the changes
> between 2.871 and 2.873.  Also one of the intermediate versions got
> complaints due to that code path not working correctly.

I'm totally willing to retry.  A real mirror, even a local one, won't
have any problems because it has a setup.ini.

It would be pretty neat if genini could be simplified(*), so a mortal user
can just run it with a directory as parameter and it creates a setup.ini
file at the top-level of that dir without too much complaining about
missing sources etc.

(*) Or better: A user version of calm, packaged in the distro.


Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: setup and colons in filenames

[Achim Gratz]

Corinna Vinschen writes:
> It would be pretty neat if genini could be simplified(*), so a mortal user
> can just run it with a directory as parameter and it creates a setup.ini
> file at the top-level of that dir without too much complaining about
> missing sources etc.
>
> (*) Or better: A user version of calm, packaged in the distro.

For better or worse, genini is dead and replaced by mksetupini.  The
latter is way more picky with having the correct hint files around and
often refuses to create a setup.ini if it finds something it doesn't
like.


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Wavetables for the Terratec KOMPLEXER:
http://Synth.Stromeko.net/Downloads.html#KomplexerWaves

Re: setup and colons in filenames

[Jon Turney]

On 26/10/2017 19:25, Achim Gratz wrote:
> Jon Turney writes:
>> Achim Gratz wrote:
>>> Jon Turney writes:
>>>> This seems a really odd thing to do, as we've no idea about the
>>>> dependencies of these packages, so installing them is unlikely work
>>>> well.
>>>
>>> This particular code path was vetoed from getting thrown out last time I
>>> worked in that area since it would break long-standing expectations
>>> w.r.t. local package archives that have no setup.ini or even *.hint. 
>>
>>> Can we have a link to that discussion, please?
>> 
>> It's somewhere in that thread from June 2015 related to the changes
>> between 2.871 and 2.873.  Also one of the intermediate versions got
>> complaints due to that code path not working correctly.

Using your clues, the only seemingly relevant discussion I found was 
starting at [1], but I can't find where it says anything like that.

https://cygwin.com/ml/cygwin-apps/2015-06/msg00091.html

Re: setup and colons in filenames

[Jon Turney]

On 27/10/2017 11:40, Achim Gratz wrote:
> Corinna Vinschen writes:
>> It would be pretty neat if genini could be simplified(*), so a mortal user
>> can just run it with a directory as parameter and it creates a setup.ini
>> file at the top-level of that dir without too much complaining about
>> missing sources etc.

I've worked on [1] a bit so it now contains some instructions how to do 
this, rather than the previous state of affairs, which was "genini 
exists, work it out yourself"

[1] https://cygwin.com/package-server.html#overlay

>> (*) Or better: A user version of calm, packaged in the distro.

Um, we have had this for a while [2], mksetupini is the tool to use if 
you don't need all the extra stuff which calm does.

[2] https://cygwin.com/ml/cygwin-apps/2016-07/msg00047.html

I guess mksetupini could have more friendly defaults to make the command 
lines in [1] shorter if that really scares people off (e.g. default 
--inifile to stdout, --releasearea to '.' and to 
--okmissing=required-package (then need a way to turn it back on, though))

> For better or worse, genini is dead and replaced by mksetupini.  The
> latter is way more picky with having the correct hint files around and
> often refuses to create a setup.ini if it finds something it doesn't
> like.

Not dead, just resting.  Until someone patches it to understand pvr.hint 
files.

I make no apologies for mksetupini being more strict, though.

Re: setup and colons in filenames

[Ken Brown]

On 10/27/2017 9:44 AM, Jon Turney wrote:
> I've worked on [1] a bit so it now contains some instructions how to do 
> this, rather than the previous state of affairs, which was "genini 
> exists, work it out yourself"
> 
> [1] https://cygwin.com/package-server.html#overlay
> 
>>> (*) Or better: A user version of calm, packaged in the distro.
> 
> Um, we have had this for a while [2], mksetupini is the tool to use if 
> you don't need all the extra stuff which calm does.
> 
> [2] https://cygwin.com/ml/cygwin-apps/2016-07/msg00047.html
> 
> I guess mksetupini could have more friendly defaults to make the command 
> lines in [1] shorter if that really scares people off (e.g. default 
> --inifile to stdout, --releasearea to '.' and to 
> --okmissing=required-package (then need a way to turn it back on, though))

I don't see why this should be necessary.  You've written out the 
command line for people to copy and paste.

With [1] available, I think it should be OK to rip out ScanFindVisitor.

Ken

Re: setup and colons in filenames

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 1288 bytes --]

On Oct 27 11:20, Ken Brown wrote:
> On 10/27/2017 9:44 AM, Jon Turney wrote:
> > I've worked on [1] a bit so it now contains some instructions how to do
> > this, rather than the previous state of affairs, which was "genini
> > exists, work it out yourself"
> > 
> > [1] https://cygwin.com/package-server.html#overlay
> > 
> > > > (*) Or better: A user version of calm, packaged in the distro.
> > 
> > Um, we have had this for a while [2], mksetupini is the tool to use if
> > you don't need all the extra stuff which calm does.
> > 
> > [2] https://cygwin.com/ml/cygwin-apps/2016-07/msg00047.html
> > 
> > I guess mksetupini could have more friendly defaults to make the command
> > lines in [1] shorter if that really scares people off (e.g. default
> > --inifile to stdout, --releasearea to '.' and to
> > --okmissing=required-package (then need a way to turn it back on,
> > though))
> 
> I don't see why this should be necessary.  You've written out the command
> line for people to copy and paste.
> 
> With [1] available, I think it should be OK to rip out ScanFindVisitor.
> 
> Ken

ACK


Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]