> On what OS/arch does this happen?

Thank you. The test failure is occurring on my(our) customized OS.
If I find an issue with the code, I will report it. However, at this point,
I don't think there is any problem with LibreSSL.

> I'd suggest we keep this part of your diff:

Your suggestion looks good to me. I would like you to merge the changes.

Regards,
Kenjiro


On Sun, Jul 21, 2024 at 7:29 PM Theo Buehler <tb@theobuehler.org> wrote:

> On Sun, Jul 21, 2024 at 07:10:01PM +0900, Kenjiro Nakayama wrote:
> > When one of the asn1_time_compare_test fails, it is difficult to
> > determine which test failed.
> >
> > For example, currently it just outputs:
> >
> > ```
> > $ ./tests/asn1time
> >   ... (snip)...
> > ASN1_TIME_compare tests...
> > 140524144512Z vs. 150923032700Z: want -1, got 0
>
> On what OS/arch does this happen?
>
> > Time overflow tests...
> > ```
> >
> > Therefore, this patch adds some logs to make debugging easier.
>
> We can add a bit of info, but...
>
> >
> >                       if (time_cmp != asn1_cmp) {
> > -                             fprintf(stderr, "%s vs. %s: want %d, got
> %d\n",
> > -                                 att1->str, att2->str, time_cmp,
> asn1_cmp);
> > +                             fprintf(stderr, "test %d
> (ASN1_TIME_compare) - %s vs. %s: want %d, got %d\n",
> > +                                 i, att1->str, att2->str, time_cmp,
> asn1_cmp);
> >                               comparison_failure |= 1;
>
> I don't think it is super helpful to add 'test %d' to the output.
> It is easy to determine what table the 'att1->str came from.
>
> > @@ -624,9 +624,13 @@ asn1_time_compare_test(void)
> >       size_t utc_size = N_UTCTIME_TESTS;
> >       int failed = 0;
> >
> > +     fprintf(stderr, "Time compare tests (gen, gen)...\n");
> >       failed |= asn1_time_compare_families(gen, gen_size, gen, gen_size);
> > +     fprintf(stderr, "Time compare tests (gen, utc)...\n");
> >       failed |= asn1_time_compare_families(gen, gen_size, utc, utc_size);
> > +     fprintf(stderr, "Time compare tests (utc, gen)...\n");
> >       failed |= asn1_time_compare_families(utc, utc_size, gen, gen_size);
> > +     fprintf(stderr, "Time compare tests (utc, utc)...\n");
> >       failed |= asn1_time_compare_families(utc, utc_size, utc, utc_size);
>
> And this is too much in my opinion. The test is already too noisy.
>
> I'd suggest we keep this part of your diff:
>
> Index: asn1time.c
> ===================================================================
> RCS file: /cvs/src/regress/lib/libcrypto/asn1/asn1time.c,v
> diff -u -p -r1.29 asn1time.c
> --- asn1time.c  25 May 2024 18:59:03 -0000      1.29
> +++ asn1time.c  21 Jul 2024 10:25:18 -0000
> @@ -581,14 +581,16 @@ asn1_time_compare_families(const struct
>                         asn1_cmp = ASN1_TIME_compare(t1, t2);
>
>                         if (time_cmp != asn1_cmp) {
> -                               fprintf(stderr, "%s vs. %s: want %d, got
> %d\n",
> +                               fprintf(stderr, "ASN1_TIME_compare - %s
> vs. %s: "
> +                                   "want %d, got %d\n",
>                                     att1->str, att2->str, time_cmp,
> asn1_cmp);
>                                 comparison_failure |= 1;
>                         }
>
>                         time_cmp = ASN1_TIME_cmp_time_t(t1, att2->time);
>                         if (time_cmp != asn1_cmp) {
> -                               fprintf(stderr, "%s vs. %lld: want %d, got
> %d\n",
> +                               fprintf(stderr, "ASN1_TIME_cmp_time_t - %s
> vs. %lld: "
> +                                   "want %d, got %d\n",
>                                     att1->str, (long long)att2->time,
>                                     asn1_cmp, time_cmp);
>                                 comparison_failure |= 1;
> @@ -598,7 +600,8 @@ asn1_time_compare_families(const struct
>                         if (t1->type != V_ASN1_UTCTIME)
>                                 asn1_cmp = -2;
>                         if (time_cmp != asn1_cmp) {
> -                               fprintf(stderr, "%s vs. %lld: want %d, got
> %d\n",
> +                               fprintf(stderr, "ASN1_UTCTIME_cmp_time_t -
> %s vs. %lld: "
> +                                   "want %d, got %d\n",
>                                     att1->str, (long long)att2->time,
>                                     asn1_cmp, time_cmp);
>                                 comparison_failure |= 1;
>


-- 
Kenjiro NAKAYAMA <nakayamakenjiro@gmail.com>
GPG Key fingerprint = ED8F 049D E67A 727D 9A44  8E25 F44B E208 C946 5EB9