Re: [RELEASE CANDIDATE] mod_perl-2.0.11 RC2

On Thu, 26 Sep 2019 at 06:54, Damyan Ivanov <[email protected]> wrote:
>
> -=| Damyan Ivanov, 25.09.2019 23:06:56 +0300 |=-
> > -=| Steve Hay, 25.09.2019 08:34:07 +0100 |=-
> > > What happens if you revert the change in that file? I.e. Change
> > > FINFO_NAME back to FINFO_NORM on line 168, and on line 18 (the
> > > APR::Const -compile line)? Does that fix it for you?
> >
> > With these two lines reverted, all tests pass for me.
>
> Out of interest I also tried using "FINFO_NAME | FINFO_NORM" on line
> 168 and the test failed with the same error. It appears that
> FINFO_NAME triggers it.
>
> Tracing this in APR, the error seems to come from
> file_io/unix/filestat.c¹ where the valid flags on line 73 don't
> include APR_FINFO_NAME. This kind of makes sense, because the stat(2)
> struct lacks information about the file name.
>
>  ¹ https://sources.debian.org/src/apr/1.6.5-1/file_io/unix/filestat.c/
>
> -- Damyan

Good catch! That explains why using FINFO_NAME is failing for you but
working for me (on Windows): apr/file_io/win32/filestat.c *does* have
support for FINFO_NAME in it, filling in finfo->name from filename if
it succeeded in getting filename (using calls other than stat()!).

The point of the request_rec test in question is only to test a single
field anyway (the rest are tested in TestAPR::finfo, as the comment
says) so I will simply change it to test a different field. Evidently
FINFO_NAME was a poor choice!

Before I actually roll out an RC3, please can you confirm that
switching to FINFO_SIZE as per this patch works for you (it does for
me):

Index: t/response/TestAPI/request_rec.pm
===================================================================
--- t/response/TestAPI/request_rec.pm (revision 1866274)
+++ t/response/TestAPI/request_rec.pm (working copy)
@@ -15,7 +15,7 @@
 use APR::Pool ();

 use Apache2::Const -compile => qw(OK M_GET M_PUT);
-use APR::Const    -compile => qw(FINFO_NAME);
+use APR::Const    -compile => qw(FINFO_SIZE);

 #this test module is only for testing fields in the request_rec
 #listed in apache_structures.map
@@ -165,12 +165,13 @@

     # finfo
     {
-        my $finfo = APR::Finfo::stat(__FILE__,
APR::Const::FINFO_NAME, $r->pool);
+        my $size = (stat __FILE__)[7];
+        my $finfo = APR::Finfo::stat(__FILE__,
APR::Const::FINFO_SIZE, $r->pool);
         $r->finfo($finfo);
         # just one field test, all accessors are fully tested in
         # TestAPR::finfo
-        ok t_cmp($r->finfo->fname,
-                 __FILE__,
+        ok t_cmp($r->finfo->size,
+                 $size,
                  '$r->finfo');
     }

Index: t/response/TestAPI/request_rec.pm
===================================================================
--- t/response/TestAPI/request_rec.pm	(revision 1866274)
+++ t/response/TestAPI/request_rec.pm	(working copy)
@@ -15,7 +15,7 @@
 use APR::Pool ();
 
 use Apache2::Const -compile => qw(OK M_GET M_PUT);
-use APR::Const    -compile => qw(FINFO_NAME);
+use APR::Const    -compile => qw(FINFO_SIZE);
 
 #this test module is only for testing fields in the request_rec
 #listed in apache_structures.map
@@ -165,12 +165,13 @@
 
     # finfo
     {
-        my $finfo = APR::Finfo::stat(__FILE__, APR::Const::FINFO_NAME, $r->pool);
+        my $size = (stat __FILE__)[7];
+        my $finfo = APR::Finfo::stat(__FILE__, APR::Const::FINFO_SIZE, $r->pool);
         $r->finfo($finfo);
         # just one field test, all accessors are fully tested in
         # TestAPR::finfo
-        ok t_cmp($r->finfo->fname,
-                 __FILE__,
+        ok t_cmp($r->finfo->size,
+                 $size,
                  '$r->finfo');
     }