Re: [blead 2009-05-29.22:08:17] vms.c EFS logical name fix.

Craig A. Berry wrote:
> 
> On Jun 8, 2009, at 1:42 AM, John E. Malmberg wrote:
Below inline:
>> John E. Malmberg wrote:
>>> Craig A. Berry wrote:
>>>>
>>>> I went back to my original test case (not the narrower one you ended 
>>>> up fixing) and it looks like we are half-way there.  If both 
>>>> DECC$EFS_CHARSET and DECC$FILENAME_UNIX_REPORT are defined, this works:
>>>>
>>>> $ perl -"MFile::Spec::Functions" -e "print 
>>>> catfile(File::Spec->tmpdir(), 'bar');"
>>>> /sys$scratch/bar
>>>>
>>>> but if only DECC$EFS_CHARSET is defined, it doesn't:
>>>>
>>>> $ perl -"MFile::Spec::Functions" -e "print 
>>>> catfile(File::Spec->tmpdir(), 'bar');"
>>>> [.sys$scratch:]bar
>>> The attached patch has not been run through a full test run yet.
>>> The problem was that catfile was expecting that the path components 
>>> were either contained VMS directory delimiters or were bare words.
>>> The earlier part of the code detected the trailing ":" so knew to 
>>> treat the path a VMS.
>>> This patch allows a path ending with a ":" to be treated as a VMS 
>>> directory path.
>>
>> This patch is better, but still not fully tested.
> 
> Sounds good.  I'll try to take it for a spin sometime soon.  We also 
> need to sort out whether blead is a head of File::Path on CPAN or vice 
> versa and get synched up.
> 
>> I did not run a comprehensive test of EFS mode with out Unix report 
>> also on before, and this is exposing some dragons that are going to 
>> take some time to slay.
> 
> I suppose one way to deal with that is to recommend they be used 
> together for now.
>>
>> There are a number of tests and modules that will need to be adjusted 
>> to handle VMS EFS charsets that I have found.
>>
>> testp2pt.pl needs a patch as it is trying to do catfile() on directories.
>>
>> cwd.t is creating an illegal symbolic link unless it is in UNIX mode, 
>> so it does not get resolved.
>>
>> ext/File-Glob/t/Basic.t is trying to compare the results of a 
>> directory in UNIX syntax with one in VMS syntax.  With EFS on, VMS 
>> names with out extensions have the trailing periods present.
>>
>> There are a few other failures that I do not understand yet.
>>
>> Regards,
>> -John
>> [email protected]
>> Personal Opinion Only
>> --- /rsync_root/perl/lib/File/Spec/VMS.pm    Sun May 10 04:02:09 2009
>> +++ lib/File/Spec/VMS.pm    Mon Jun  8 01:19:01 2009
>> @@ -195,6 +195,32 @@
>>
>>         if ($efs) {
>>             # Extended character set in use, go into DWIM mode.
>> +            if ($path =~ /:$/) {
>> +                # We have a problem.  If we do a syntax only conversion
>> +                # we can break existing code that does not understand 
>> how
>> +                # logical names and directories can be combined.
>> +                my $test_path = $path;
>> +                my $i = 0;
>> +                while ($i < 10) {
>> +                    # We need to do a logical name check, but we do 
>> not have
>> +                    # logical name services in perl.  So we need to 
>> $ENV until
>> +                    # that can be remedied.
>> +                    $test_path =~ s/:$//;
>> +                    my $test_trnlnm = $ENV{$test_path};
>> +                    last unless defined $test_trnlnm;
>> +                    if ($test_trnlnm =~ /\.[\]>]$/) {
>> +                        # A rooted logical name, same as device so 
>> use as is.
>> +                        last;
>> +                    }
>> +                    if ($test_trnlnm =~ /[\]>]$/) {
>> +                        # Found a directory, use this instead of the 
>> path
>> +                        $path = $test_trnlnm;
>> +                        last;
>> +                    }
>> +                    $test_path = $test_trnlnm;
>> +                    $i++;
>> +                };
>> +            }
> 
> I don't understand the rationale for this.  I think we want "foo:" in 
> "foo:bar" treated like a device name whether foo is a logical name or 
> not.  I don't think that would be a legal Unix specification, at least 
> not without the colon being escaped, so I don't think we should work too 
> hard to allow for the rare (maybe impossible) case where the colon is 
> supposed to be just another filename character.

Existing library code is using catdir('sys$scratch:','tempdir').  The 
above code is needed for that to work.

I am not sure if I need to do something to handle if tainted is on.

>>       }
>>
>> @@ -429,6 +469,7 @@
>>                 my $spath_vms = 0;
>>                 $spath_vms = 1 if ($spath =~ m#[\[<\]]#);
>>                 $spath_vms = 1 if ($spath =~ /^--?$/);
>> +                $spath_vms = 1 if ($spath =~ /:$/);
> 
> Some thought needs to be given to performance in code like this.  I 
> think those four lines could be replaced with
> 
> my $spath_vms = $spath eq '--' || ($spath =~ m#([\[<\]]|:$)#);
> 
> There is no reason to use a regex when you are matching the entire 
> string, so replace C<$spath =~ /^--?$/> with C<$spath eq '--'>.  If one 
> check passes, we should stop checking and not do the other checks.  
> Combining the first and third regexes into one should be more efficient 
> (there are further improvements that could be made to that regex).

It also has to handle the case of $spath = '-' or $spath = '---' or 
spath consisting of any number of '-' characters.

The other optimization should work.

I just kicked off a test run with the previously attached patch and will 
check on it after work to see how many tests fail.

Regards,
-John
[email protected]
Personal Opinion Only