Opened 5 years ago

Closed 5 years ago

#3513 closed enhancement (complete)

-V (extended version) should return more details

Reported by: tomek Owned by: fdupont
Priority: low Milestone: Kea0.9.2-beta
Component: dhcp Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

In ticket #3508, a -v (version number) and -V (extended version info) command-line switches have been added.

We should added extra details to -V: if compiled with MySQL, Postgres, Botan or OpenSSL, which Boost version is used, etc.

This will be useful for debugging purposes.

Also, as Tom suggested in #3508 comment 7, unit-tests for -v need some improvement.

Subtickets

Change History (35)

comment:1 Changed 5 years ago by tomek

  • Milestone changed from Kea-proposed to Kea1.0
  • Version set to git

comment:2 Changed 5 years ago by tomek

  • Component changed from Unclassified to dhcp

comment:3 Changed 5 years ago by tomek

  • Milestone changed from Kea1.0 to Kea0.9.1

comment:4 Changed 5 years ago by fdupont

How it is done in bind9:
-v
%s %s

  • ns_g_product (field PRODUCT in ./version)
  • ns_g_version (built by configure from various fields in ./version) %s
  • (optional) ns_g_description (field DESCRIPTION in ./version)

-V

id:%s> built by %s with %s

  • ns_g_srcid (from ./srcid or git)
  • ns_g_builder (from Makefile)
  • ns_g_configargs (built by configure from $ac_configure_args) compiled by XXX %s
  • ifdefs and VERSION & co

using OpenSSL version: OPENSSL_VERSION_TEXT
using libxml2 version: LIBXML_DOTTED_VERSION

An extra complexity for bind9 comes from the VS/WIN32 build: it can't use shell and configure stuff so the correspondent code is in the perl win32utils/Configure script (I wrote it so I more than know it :-).

Current in KEA:
-v / VERSION : PACKAGE_VERSION from configure AC_INIT
-V / EXTENDED_VERSION : KEA_SRCID from git & co
note the configure has a config.h EXTENDED_VERSION and a *different* variable displayed in the summary == VERSION (KEA_SRCID)

Proposal:

  • unify config.h and configure summary stuff
  • introduce configure, compiler, boost, crypto, log, config and sql config.h stuff (as explained by Tomek not all are pertinent for all tools but if we have the feature for configure summary (i.e., nothing really new to do) and it has an impact for a command then it should be displayed by -V).

comment:5 Changed 5 years ago by hschempf

  • Milestone changed from Kea0.9.1 to Kea0.9.2

comment:6 Changed 5 years ago by hschempf

  • Priority changed from medium to low

comment:7 Changed 5 years ago by fdupont

  • Owner set to fdupont
  • Status changed from new to accepted

comment:8 Changed 5 years ago by fdupont

BTW there can be a difference between the version Kea was configured with and the version Kea runs with. This particular point was known to be critical for OpenSSL which did some arguable API changes (this is BTW the official reason used by Apple to try to deprecate OpenSSL in recent OS X's).

comment:9 Changed 5 years ago by fdupont

I note to check the config.report is part of the distrib (I think so).

comment:10 Changed 5 years ago by fdupont

PS: a drastic solution is to compile in the config.report. It can be a reasonable alternative to produce a long report on -V.

comment:11 Changed 5 years ago by fdupont

Nearly done (the only remaining point is to update the documentation).
The magic command to get the embedded config.report from kea-dhcp{4,6,-ddns}, perfdhcp and kea-lfc is

strings <binary> | sed -n 's/;;;; //p'

comment:12 Changed 5 years ago by fdupont

Branch ready. Here is a proposal for the ChangeLog which could be improved:
The extended version -V command now displays the log4cplus and crypto backend dynamic/runtime library versions, and the kind of database used.
The config.report is embedded into executable binaries to provide a way to get static/configuration information.

Note I don't detail the MySQL/PostgreSQL linked library versions. If they are useful (and a priori they are) we should open a followup ticket to finish this in a further release...

comment:13 Changed 5 years ago by fdupont

  • Owner changed from fdupont to UnAssigned
  • Status changed from accepted to reviewing

comment:14 Changed 5 years ago by fdupont

Missing changes in tests/Makefile.am files...

comment:15 Changed 5 years ago by fdupont

  • Owner changed from UnAssigned to fdupont
  • Status changed from reviewing to accepted

comment:16 Changed 5 years ago by fdupont

Fixed, ready again for review.

comment:17 Changed 5 years ago by fdupont

  • Owner changed from fdupont to UnAssigned
  • Status changed from accepted to reviewing

comment:18 Changed 5 years ago by fdupont

I am creating a followup giving the database details:

  • name of the backend (as in #3513)
  • version of the backend (defined in xxx_lease_mgr.h)
  • version of the library:
    • memfile: nothing
    • mysql: mysql_get_client_info()
    • postgresql: pgsql_version()

comment:19 Changed 5 years ago by fdupont

Followup #3882. Still under review (and medium priority #3859 depends on this).

comment:20 follow-up: Changed 5 years ago by sar

  • Owner changed from UnAssigned to fdupont

Is there a reason to put the cfgrpt stuff in src/bin instead of src/lib? It seems to be more of a library than a binary. Also I find the idea of using the strings command to get the information a bit off-putting, Is there a reason beyond having a long output to not have -V simply print all of the report?

Neither dhcp4 or dhcp6 seem to be using the crypto link library yet, are we just including it to get the version information?

bin/dhcp6/dhcp6_srv.cc

There are two copies of "tmp << end << EXTENDED_VERSION"

d2, dhcp4 and dhcp6 binaries

For the d2, dhcp4 and dhcp6 binaries I would change the output slightly. I would put "linked with:" on one line and the other items on succeeding lines with no "and". Also, as the code can be compiled for multiple databases, I would move the databases on to one line (though that may not work with the other tickets).
This would look something like this:

0.9.1-git
git f78c80113a660573dc3e701d0eb06faaa150df71
linked with:
log4plus 1.1.2
Botan 1.10.8 (released 20140410, revision mtn:3e5da04321de05a3ae1f7177e0dd2191e598dca7, distribution unspecified)
database(s): memfile, mySQL, PostgresSQL

I imagine the database code would be something like:

tmp << "database(s): memfile";
#ifdef HAVE_MYSQL
tmp << ", MySQL";
#endif
#ifdef HAVE_PGSQL
tmp << ", PostgresSQL";
#endif
tmp << endl;

xml text

I would rewrite the xml text as something more like this:

All of the executable binaries in Kea contain an embedded copy of the <filename>config.report</filename> produced by <userinput>./configue</userinput>. The following command may be used to extract this information. The binary <userinput>path</userinput>, may be found in the install directory or in the <filename>.libs</filename> subdirectory in the source treee. For example <filename>kea/src/bin/d2/.libs/kea-dhcp-ddns</filename>

<insert example command>

comment:21 in reply to: ↑ 20 ; follow-up: Changed 5 years ago by fdupont

Replying to sar:

Is there a reason to put the cfgrpt stuff in src/bin instead of src/lib? It seems to be more of a library than a binary.

=> it is not a shared library so IMHO it is better to mark the difference (BTW its interest lies in the fact it is not a shared library).

Also I find the idea of using the strings command to get the information a bit off-putting,

=> it is pretty standard (I remember to have seen this before).

Is there a reason beyond having a long output to not have -V simply print all of the report?

=> the best should be to have 3 options. Note the report must be better formatted and in the long term if we follow this way. IMHO my proposal is enough as the information in the report is useful only for advanced users, i.e., people who already use strings and co to get embedded version strings...

Neither dhcp4 or dhcp6 seem to be using the crypto link library yet, are we just including it to get the version information?

=> they use the DNS++ library which use the cryptolink library. Unfortunately library issues are not limited to directly linked libraries.

bin/dhcp6/dhcp6_srv.cc

There are two copies of "tmp << end << EXTENDED_VERSION"

=> fixed

d2, dhcp4 and dhcp6 binaries

For the d2, dhcp4 and dhcp6 binaries I would change the output slightly. I would put "linked with:" on one line and the other items on succeeding lines with no "and".

=> done.

Also, as the code can be compiled for multiple databases, I would move the databases on to one line (though that may not work with the other tickets).

=> this is addressed in #3882.

I imagine the database code would be something like:

=> in fact simpler (no comma issue) with the memfile at the end (it is not optional) without an end of line (added by the caller).

xml text

I would rewrite the xml text as something more like this:

=> yes, it is better. Adopted!

comment:22 Changed 5 years ago by fdupont

I pushed the changes for this ticket and #3882. I still have to check if the code still compiles and returns what is expected but I release the ticket for re-review. BTW please don't forget some tickets depend on this one.

comment:23 Changed 5 years ago by fdupont

  • Owner changed from fdupont to sar

comment:24 in reply to: ↑ 21 ; follow-up: Changed 5 years ago by sar

  • Owner changed from sar to fdupont

Replying to fdupont:

Replying to sar:

<snip>

Also I find the idea of using the strings command to get the information a bit off-putting,

=> it is pretty standard (I remember to have seen this before).

Is there a reason beyond having a long output to not have -V simply print all of the report?

=> the best should be to have 3 options. Note the report must be better formatted and in the long term if we follow this way. IMHO my proposal is enough as the information in the report is useful only for advanced users, i.e., people who already use strings and co to get embedded version strings...

I chatted with Tomek and we both think that it would be better to include the third option to print out the config report information in this ticket. I think it should be pretty simple - create a routine to print out the strings and add calls to that routine in the affected programs. The suggested option was "-VV" or --extra-verbose but another option could be suggested as well.

One of the reasons for this work and this entire ticket in general is to provide a convenient way for users to collect the information we may need in order to diagnose any problems that occur. The easier we make that the more likely we will be able to get the user to supply us the correct information - having a simple option is likely to be much better than requiring the user to do strings and grep.

Also, as the code can be compiled for multiple databases, I would move the databases on to one line (though that may not work with the other tickets).

=> this is addressed in #3882.

OK - it will get reviewed as part of that ticket - we shall figure out which milestone that ticket goes into shortly.

xml text

I would rewrite the xml text as something more like this:

=> yes, it is better. Adopted!

If we add a -VV option then this text will need a small further revision.

comment:25 in reply to: ↑ 24 Changed 5 years ago by fdupont

=> the best should be to have 3 options. Note the report must be better formatted and in the long term if we follow this way. IMHO my proposal is enough as the information in the report is useful only for advanced users, i.e., people who already use strings and co to get embedded version strings...

I chatted with Tomek and we both think that it would be better to include the third option to print out the config report information in this ticket. I think it should be pretty simple - create a routine to print out the strings and add calls to that routine in the affected programs. The suggested option was "-VV" or --extra-verbose but another option could be suggested as well.

=> I believe we'd like to keep the simple getup so one letter and no extended stuff. As there is no German here I suggest -W (in French the W is the "double V", in German it is a double U :-). Note I prefer to keep the info embedded in the current/proposed way as it is easy to extract without launching a Kea binary (i.e., static is static, dynamic is not).

One of the reasons for this work and this entire ticket in general is to provide a convenient way for users to collect the information we may need in order to diagnose any problems that occur. The easier we make that the more likely we will be able to get the user to supply us the correct information - having a simple option is likely to be much better than requiring the user to do strings and grep.

=> it is a matter of taste but as it is not hard to add now (it will take far more time to build something to test than to write the code :-).

Also, as the code can be compiled for multiple databases, I would move the databases on to one line (though that may not work with the other tickets).

=> this is addressed in #3882.

If we add a -VV option then this text will need a small further revision.

=> of course.

comment:26 Changed 5 years ago by fdupont

Done: now the config.report file is displayed by -W for the 3 kea-dhcp*.
I tried to improve the doc too but I am afraid it will need another pass (please commit the doc changes directly to the branch).

Last edited 5 years ago by fdupont (previous) (diff)

comment:27 Changed 5 years ago by fdupont

  • Owner changed from fdupont to UnAssigned

comment:28 Changed 5 years ago by sar

  • Owner changed from UnAssigned to sar

comment:29 follow-up: Changed 5 years ago by sar

  • Owner changed from sar to fdupont

The changes look fine.

I've updated the ddns document doc/guide/ddns.xml to (I think) better describe the -W command. If you agree with the changes they should get copied and pasted into the dhcp4 and dhcp6 documents.

I noticed that the usage strings for dhcp4 and dhcp6 are a bit different with their naming of the config file (4 => [-c file] at the end and => [-c cfg file] at the beginning. If there isn't a reason for the difference please pick one (I favor the dhcp6 version) and use it for both.

comment:30 in reply to: ↑ 29 Changed 5 years ago by fdupont

Replying to sar:

I've updated the ddns document doc/guide/ddns.xml to (I think) better describe the -W command. If you agree with the changes they should get copied and pasted into the dhcp4 and dhcp6 documents.

=> done

I noticed that the usage strings for dhcp4 and dhcp6 are a bit different with their naming of the config file (4 => [-c file] at the end and => [-c cfg file] at the beginning. If there isn't a reason for the difference please pick one (I favor the dhcp6 version) and use it for both.

=> I adopted the standard order:

  • flags (no argument) first then options (with argument)
  • in the usage the logic order, in the code the alpha order

Note an alternative is to use strict alpha order, IMHO better when there are many flags and options, i.e., when they can't stand in one line (which is not the case here).

Ready for another round (but I'll wait for #3859 and #3882 too).

comment:31 Changed 5 years ago by fdupont

  • Owner changed from fdupont to sar

comment:32 Changed 5 years ago by sar

  • Owner changed from sar to fdupont

The changes look fine.

You should merge and commit this ticket and 3859 and not wait for 3882. While 3882 is on the milestone list for 0.9.2 it is a low priority item and may not get done in time.

comment:33 Changed 5 years ago by sar

Oh, I also made a small change so please do a pull before proceeding.

comment:34 Changed 5 years ago by fdupont

Merged with #3859. Closing.

comment:35 Changed 5 years ago by fdupont

  • Resolution set to complete
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.