Re: [PATCH 2/2 v2] smime: tests of X.509 certificate validity are known-broken on GMime < 3.2.7

Subject: Re: [PATCH 2/2 v2] smime: tests of X.509 certificate validity are known-broken on GMime < 3.2.7

Date: Thu, 21 May 2020 20:41:13 -0400

To: David Bremner, Notmuch Mail


From: Daniel Kahn Gillmor

Thanks for the review, and for the poke about out-of-tree builds on IRC,
Bremner.  Another revision is coming in a minute.  Notes below…

On Thu 2020-05-21 20:29:05 -0300, David Bremner wrote:
> I find these long lines with !! in the middle pretty surprising. Is
> there some reason for this style? It doesn't seem to fit with the usual
> conventions.

Hm, do we even have conventions for inlined C in ./configure?

If you'd rather i expand these to something more verbose, i can do so,
but i was under the impression that we wanted to keep these C
interstitials fairly compact so that ./configure is still (somewhat)
readable as a shell script.

The "return !! fprintf…" idiom is a compact way to get a non-zero
process error code and an error message to stderr without introducing a
code block.  The only way for fprintf to return 0 (which would result in
the process returning 0) is if 0 bytes are written and no error
occurred, which isn't possible with any of the format strings supplied

Another approach would be to pull the C entirely out of ./configure, but
that could have a problem when dealing with out-of-tree builds.  (i just
noticed a problem for this test with out-of-tree builds, which i'll
revise in a minute)

> This line in particular has a tab in the middle.

i dunno how that got there, i'll have it fixed in the upcoming revision.

>> +    elif ${CC} ${CFLAGS} ${gmime_cflags} _check_x509_validity.c ${gmime_ldflags} -o _check_x509_validity \
> The other test files are cleaned up in configure (source and binary)
> once we are done with them.

good point, i'll ensure that they get cleaned up alongside
_check_session_keys* in the upcoming revision.

> As far as I could follow, the changes to the tests themselves look
> reasonable.

thanks for the thoughtful review!

signature.asc (application/pgp-signature)
notmuch mailing list