[Patch] Encode invalid chars in /proc/registry entries

Christian Franke Christian.Franke@t-online.de
Fri Nov 16 20:49:00 GMT 2007


Christopher Faylor wrote:
> On Fri, Nov 16, 2007 at 08:35:48PM +0100, Christian Franke wrote:
>   
>> Christopher Faylor wrote:
>>     
>>> ..
>>>       
>>>>> Patch is tested with 1.5.24-2. Merge with HEAD looks good, but was not 
>>>>> actually tested. Therefore, no changelog provided yet.
>>>>>       
>>>>>           
>>>> Thanks for this patch.  Apart from the missing ChangeLog I'm inclined
>>>> to apply it to the upcoming 1.5.25 release, but I don't like to have it
>>>> in HEAD as is.
>>>>     
>>>>         
>>> I'm not so sure it's appropriate for either yet.
>>>
>>> Isn't it possible to use at least some of the managed mode functions
>>> which deal with munging characters to do some of encoding?  It seems
>>> like the patch duplicates some of the functionality from path.cc.
>>>
>>> I realize that the registry is sort of the opposite of a managed mount
>>> but it seems like the encoding functions might be potentially used in
>>> reverse for this.
>>>       
>> I actually consulted path.cc before starting the patch but did not find
>> any function which provides the required functionality OOTB.
>> Therefore, I solved the tradeoff between "reuse" and "do not change
>> working code if you don't have time for thorough regression testing" by
>> the latter :-)
>>     
>
> I'm sorry but "reuse" is a fairly important concept in a project like
> this.  

Agree. But there is always this tradeoff when fixing bugs.


> The proc functions, in particular, have been prone to NIH and I
> don't want to see even more there if we can possibly help it.
>
>   

> So, I'll reiterate my suggestion that you look at, e.g.,
> mount_item::fnmunge and possibly think about generalizing it if it isn't
> quite up to the task.
>
>   

OK, I will do this for a possible HEAD patch. But not for this patch, 
due to increased regression risk.

fnmunge() is different: The chars '/' and '\' are not considered special 
but special names are handled.
The inverse fnunmunge() decodes each %XX sequence, which I didn't want 
to avoid introducing alias names.


> I'll also go on record as advocating that this not be part of a bugfix
> release.  It seems too much like a last minute change to me.
>
>   

Sorry, but I disagree. My patch does not introduce a nice-to-have 
feature. Returning non-POSIX conforming names in readdir() is IMO not a 
minor issue. And Cygwin adds the worst possible registry entry name 
("/") itself.

To be convinced, please try

$ find /proc/registry/HKEY_LOCAL_MACHINE/SOFTWARE/Cygnus\ Solutions 
 >some.file

(Own risk, it may result in a bluescreen on XP SP2)

This should be fixed in a bugfix release when a reasonable tested patch 
with low regression-risk is available (which is IMO the case now :-)


> Getting it into cvs main, however, seems like a good idea.
>
>   

Agree :-)


Christian



More information about the Cygwin-patches mailing list