From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp57.i.mail.ru (smtp57.i.mail.ru [217.69.128.37]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id E90EC430408 for ; Wed, 12 Aug 2020 15:59:33 +0300 (MSK) Received: by smtp57.i.mail.ru with esmtpa (envelope-from ) id 1k5qLt-000652-6v for tarantool-patches@dev.tarantool.org; Wed, 12 Aug 2020 15:59:33 +0300 Received: by mail-lf1-f47.google.com with SMTP id x24so1083782lfe.11 for ; Wed, 12 Aug 2020 05:59:33 -0700 (PDT) MIME-Version: 1.0 References: <20200806131955.3400088-1-yaroslav.dynnikov@tarantool.org> <4a43127c-6e11-8f28-27ee-62e24fa65a4b@tarantool.org> In-Reply-To: <4a43127c-6e11-8f28-27ee-62e24fa65a4b@tarantool.org> From: Yaroslav Dynnikov Date: Wed, 12 Aug 2020 15:59:21 +0300 Message-ID: Content-Type: multipart/alternative; boundary="0000000000004db27605acadc2cd" Subject: Re: [Tarantool-patches] [PATCH] Ensure all curl symbols are exported List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: Yaroslav Dynnikov , tml , Alexander Turenko --0000000000004db27605acadc2cd Content-Type: text/plain; charset="UTF-8" Hi, Vlad. Number 7 is the libcurl age. According to the documentation, age should be set to the version of this functionality by the time you write your program. This way, libcurl will always return a proper struct that your program understands, while programs in the future might get a different struct. See https://curl.haxx.se/libcurl/c/curl_version_info.html So it's safe enough to rely on the structure. And I think both checkes (libssl and libz) are important enough to be tracked by tests. We don't want them to be silently gone, do we? Best regards Yaroslav Dynnikov On Tue, 11 Aug 2020 at 01:49, Vladislav Shpilevoy wrote: > Hi! Thanks for the patch! > > > diff --git a/test/box-tap/gh-5223-curl-exports.test.lua > b/test/box-tap/gh-5223-curl-exports.test.lua > > new file mode 100755 > > index 000000000..300d60b07 > > --- /dev/null > > +++ b/test/box-tap/gh-5223-curl-exports.test.lua > > @@ -0,0 +1,177 @@ > > +#!/usr/bin/env tarantool > > + > > +local tap = require('tap') > > +local ffi = require('ffi') > > +ffi.cdef([[ > > + void *dlsym(void *handle, const char *symbol); > > + struct curl_version_info_data { > > 1. I don't think you need to copy-paste the entire structure. Anyway it is > returned from curl_version_info() by pointer, so you can just announce the > pointer type. And in the test check that ffi.C.curl_version_info(7) > returned > something not nil. I don't think you need to access any fields of this > struct. It is going to lead to UB in case the struct will change anyhow in > the sources. > > > + int age; /* see description below */ > > + const char *version; /* human readable string */ > > + unsigned int version_num; /* numeric representation */ > > + const char *host; /* human readable string */ > > + int features; /* bitmask, see below */ > > + char *ssl_version; /* human readable string */ > > + long ssl_version_num; /* not used, always zero */ > > + const char *libz_version; /* human readable string */ > > + const char * const *protocols; /* protocols */ > > + > > + /* when 'age' is CURLVERSION_SECOND or higher, the members > below exist */ > > + const char *ares; /* human readable string */ > > + int ares_num; /* number */ > > + > > + /* when 'age' is CURLVERSION_THIRD or higher, the members below > exist */ > > + const char *libidn; /* human readable string */ > > + > > + /* when 'age' is CURLVERSION_FOURTH or higher (>= 7.16.1), the > members > > + below exist */ > > + int iconv_ver_num; /* '_libiconv_version' if iconv > support enabled */ > > + > > + const char *libssh_version; /* human readable string */ > > + > > + /* when 'age' is CURLVERSION_FIFTH or higher (>= 7.57.0), the > members > > + below exist */ > > + unsigned int brotli_ver_num; /* Numeric Brotli version > > + (MAJOR << 24) | (MINOR << 12) | > PATCH */ > > + const char *brotli_version; /* human readable string. */ > > + > > + /* when 'age' is CURLVERSION_SIXTH or higher (>= 7.66.0), the > members > > + below exist */ > > + unsigned int nghttp2_ver_num; /* Numeric nghttp2 version > > + (MAJOR << 16) | (MINOR << 8) | > PATCH */ > > + const char *nghttp2_version; /* human readable string. */ > > + > > + const char *quic_version; /* human readable quic (+ HTTP/3) > library + > > + version or NULL */ > > + > > + /* when 'age' is CURLVERSION_SEVENTH or higher (>= 7.70.0), the > members > > + below exist */ > > + const char *cainfo; /* the built-in default > CURLOPT_CAINFO, might > > + be NULL */ > > + const char *capath; /* the built-in default > CURLOPT_CAPATH, might > > + be NULL */ > > + }; > > + > > + struct curl_version_info_data *curl_version_info(int age); > > +]]) > > + > > +local info = ffi.C.curl_version_info(7) > > 2. Why '7'? > > > +local test = tap.test('curl-features') > > +test:plan(3) > > + > --0000000000004db27605acadc2cd Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi, Vlad.

Number 7 is the libcurl = age. According to the documentation, age should be set to the version of th= is functionality by the time you write your program. This way, libcurl will= always return a proper struct that your program understands, while program= s in the future might get a different struct. See https://curl.haxx.se/libcurl/c/cur= l_version_info.html

So it's safe enough to rely on the struc= ture. And I think both checkes (libssl and libz) are important enough to be= tracked by tests. We don't want them to be silently gone, do we?
<= br>
Best reg= ards
Yaroslav Dynnikov

On Tue, 11 Aug 2020 at 01:49, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
Hi! Thanks for the= patch!

> diff --git a/test/box-tap/gh-5223-curl-exports.test.lua b/test/box-tap= /gh-5223-curl-exports.test.lua
> new file mode 100755
> index 000000000..300d60b07
> --- /dev/null
> +++ b/test/box-tap/gh-5223-curl-exports.test.lua
> @@ -0,0 +1,177 @@
> +#!/usr/bin/env tarantool
> +
> +local tap =3D require('tap')
> +local ffi =3D require('ffi')
> +ffi.cdef([[
> +=C2=A0 =C2=A0 void *dlsym(void *handle, const char *symbol);
> +=C2=A0 =C2=A0 struct curl_version_info_data {

1. I don't think you need to copy-paste the entire structure. Anyway it= is
returned from curl_version_info() by pointer, so you can just announce the<= br> pointer type. And in the test check that ffi.C.curl_version_info(7) returne= d
something not nil. I don't think you need to access any fields of this<= br> struct. It is going to lead to UB in case the struct will change anyhow in<= br> the sources.

> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 int age;=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* see description below */
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 const char *version;=C2=A0 =C2=A0 =C2=A0 = /* human readable string */
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 unsigned int version_num; /* numeric repr= esentation */
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 const char *host;=C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0/* human readable string */
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 int features;=C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0/* bitmask, see below */
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 char *ssl_version;=C2=A0 =C2=A0 =C2=A0 = =C2=A0 /* human readable string */
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 long ssl_version_num;=C2=A0 =C2=A0 =C2=A0= /* not used, always zero */
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 const char *libz_version; /* human readab= le string */
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 const char * const *protocols; /* protoco= ls */
> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 /* when 'age' is CURLVERSION_SECO= ND or higher, the members below exist */
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 const char *ares;=C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0/* human readable string */
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 int ares_num;=C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0/* number */
> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 /* when 'age' is CURLVERSION_THIR= D or higher, the members below exist */
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 const char *libidn;=C2=A0 =C2=A0 =C2=A0 = =C2=A0/* human readable string */
> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 /* when 'age' is CURLVERSION_FOUR= TH or higher (>=3D 7.16.1), the members
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0below exist */
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 int iconv_ver_num;=C2=A0 =C2=A0 =C2=A0 = =C2=A0/* '_libiconv_version' if iconv support enabled */
> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 const char *libssh_version; /* human read= able string */
> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 /* when 'age' is CURLVERSION_FIFT= H or higher (>=3D 7.57.0), the members
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0below exist */
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 unsigned int brotli_ver_num; /* Numeric B= rotli version
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (MAJ= OR << 24) | (MINOR << 12) | PATCH */
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 const char *brotli_version; /* human read= able string. */
> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 /* when 'age' is CURLVERSION_SIXT= H or higher (>=3D 7.66.0), the members
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0below exist */
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 unsigned int nghttp2_ver_num; /* Numeric = nghttp2 version
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0(MAJOR << 16) | (MINOR << 8) | PATCH */
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 const char *nghttp2_version; /* human rea= dable string. */
> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 const char *quic_version;=C2=A0 =C2=A0 /*= human readable quic (+ HTTP/3) library +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 vers= ion or NULL */
> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 /* when 'age' is CURLVERSION_SEVE= NTH or higher (>=3D 7.70.0), the members
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0below exist */
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 const char *cainfo;=C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 /* the built-in default CURLOPT_CAINFO, might
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 be N= ULL */
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 const char *capath;=C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 /* the built-in default CURLOPT_CAPATH, might
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 be N= ULL */
> +=C2=A0 =C2=A0 };
> +
> +=C2=A0 =C2=A0 struct curl_version_info_data *curl_version_info(int ag= e);
> +]])
> +
> +local info =3D ffi.C.curl_version_info(7)

2. Why '7'?

> +local test =3D tap.test('curl-features')
> +test:plan(3)
> +
--0000000000004db27605acadc2cd--