Re: [PATCH 6/8] conf, log, Makefile: Add versioning information

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 6044 bytes --]

On Fri, Oct 07, 2022 at 10:15:25AM +0200, Stefano Brivio wrote:
> On Fri, 7 Oct 2022 17:57:43 +1100
> David Gibson <david(a)gibson.dropbear.id.au> wrote:
> 
> > On Fri, Oct 07, 2022 at 02:47:40AM +0200, Stefano Brivio wrote:
> > > Now that we can log to file, this might start to be relevant.  
> > 
> > Uh... I dont' understand the connection here.
> 
> Because we might have a pile of log files collected somewhere (or
> logs shared by users) and this adds the version to the log file.

But how is that different with a system logger?

> > > Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>
> > > ---
> > >  Makefile |  3 +++
> > >  conf.c   | 12 ++++++++++--
> > >  log.c    |  4 ++--
> > >  passt.1  |  4 ++++
> > >  util.h   |  8 ++++++++
> > >  5 files changed, 27 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/Makefile b/Makefile
> > > index fafb024..f7ddb84 100644
> > > --- a/Makefile
> > > +++ b/Makefile
> > > @@ -9,6 +9,8 @@
> > >  # Copyright (c) 2021 Red Hat GmbH
> > >  # Author: Stefano Brivio <sbrivio(a)redhat.com>
> > >  
> > > +VERSION ?= $(shell git describe --tags HEAD || echo "unknown\ version")
> > > +
> > >  RLIMIT_STACK_VAL := $(shell /bin/sh -c 'ulimit -s')
> > >  ifeq ($(RLIMIT_STACK_VAL),unlimited)
> > >  RLIMIT_STACK_VAL := 1024
> > > @@ -31,6 +33,7 @@ FLAGS += -DNETNS_RUN_DIR=\"/run/netns\"
> > >  FLAGS += -DPASST_AUDIT_ARCH=AUDIT_ARCH_$(AUDIT_ARCH)
> > >  FLAGS += -DRLIMIT_STACK_VAL=$(RLIMIT_STACK_VAL)
> > >  FLAGS += -DARCH=\"$(TARGET_ARCH)\"
> > > +FLAGS += -DVERSION=\"$(VERSION)\"
> > >  
> > >  PASST_SRCS = arch.c arp.c checksum.c conf.c dhcp.c dhcpv6.c icmp.c igmp.c \
> > >  	isolation.c lineread.c log.c mld.c ndp.c netlink.c packet.c passt.c \
> > > diff --git a/conf.c b/conf.c
> > > index f22940b..4ec3153 100644
> > > --- a/conf.c
> > > +++ b/conf.c
> > > @@ -626,6 +626,8 @@ static void usage(const char *name)
> > >  	}
> > >  	info("");
> > >  
> > > +  
> > 
> > Extraneous blank line.
> 
> Oops.
> 
> > > +	info(   "  -v, --version	Show version and exit");  
> > 
> > I'd suggest just '--version'.  '-v' is "verbose" more often than it is
> > "version".
> 
> Um, yes, I'll drop -v.
> 
> > >  	info(   "  -d, --debug		Be verbose, don't run in background");
> > >  	info(   "      --trace		Be extra verbose, implies --debug");
> > >  	info(   "  -q, --quiet		Don't print informational messages");
> > > @@ -993,6 +995,7 @@ void conf(struct ctx *c, int argc, char **argv)
> > >  {
> > >  	int netns_only = 0;
> > >  	struct option options[] = {
> > > +		{"version",	no_argument,		NULL,		'v' },
> > >  		{"debug",	no_argument,		NULL,		'd' },
> > >  		{"quiet",	no_argument,		NULL,		'q' },
> > >  		{"foreground",	no_argument,		NULL,		'f' },
> > > @@ -1057,9 +1060,9 @@ void conf(struct ctx *c, int argc, char **argv)
> > >  
> > >  	if (c->mode == MODE_PASTA) {
> > >  		c->no_dhcp_dns = c->no_dhcp_dns_search = 1;
> > > -		optstring = "dqfel:hI:p:P:m:a:n:M:g:i:D:S:46t:u:T:U:";
> > > +		optstring = "vdqfel:hI:p:P:m:a:n:M:g:i:D:S:46t:u:T:U:";
> > >  	} else {
> > > -		optstring = "dqfel:hs:p:P:m:a:n:M:g:i:D:S:46t:u:";
> > > +		optstring = "vdqfel:hs:p:P:m:a:n:M:g:i:D:S:46t:u:";
> > >  	}
> > >  
> > >  	c->tcp.fwd_in.mode = c->tcp.fwd_out.mode = 0;
> > > @@ -1197,6 +1200,11 @@ void conf(struct ctx *c, int argc, char **argv)
> > >  				usage(argv[0]);
> > >  			}
> > >  			break;
> > > +		case 'v':
> > > +			fprintf(stdout,
> > > +				c->mode == MODE_PASST ? "passt " : "pasta ");
> > > +			fprintf(stdout, VERSION_BLOB);
> > > +			exit(EXIT_SUCCESS);
> > >  		case 'd':
> > >  			if (c->debug) {
> > >  				err("Multiple --debug options given");
> > > diff --git a/log.c b/log.c
> > > index 85b13fe..2b088c4 100644
> > > --- a/log.c
> > > +++ b/log.c
> > > @@ -172,7 +172,7 @@ void passt_vsyslog(int pri, const char *format, va_list ap)
> > >  }
> > >  
> > >  /**
> > > - * logfile_init() - Open log file and write header with PID and path
> > > + * logfile_init() - Open log file and write header with PID, version, path
> > >   * @name:	Identifier for header: passt or pasta
> > >   * @path:	Path to log file
> > >   * @size:	Maximum size of log file: log_cut_size is calculatd here
> > > @@ -196,7 +196,7 @@ void logfile_init(const char *name, const char *path, size_t size)
> > >  
> > >  	log_size = size ? size : LOGFILE_SIZE_DEFAULT;
> > >  
> > > -	n = snprintf(log_header, sizeof(log_header), "%s: %s (%i)",
> > > +	n = snprintf(log_header, sizeof(log_header), "%s " VERSION ": %s (%i)",
> > >  		     name, exe, getpid());
> > >  
> > >  	if (write(log_file, log_header, n) <= 0 ||
> > > diff --git a/passt.1 b/passt.1
> > > index 64236b6..c63a439 100644
> > > --- a/passt.1
> > > +++ b/passt.1
> > > @@ -77,6 +77,10 @@ for performance reasons.
> > >  
> > >  .SH OPTIONS
> > >  
> > > +.TP
> > > +.BR \-v ", " \-\-version
> > > +Show version and exit.
> > > +
> > >  .TP
> > >  .BR \-d ", " \-\-debug
> > >  Be verbose, don't run in background, don't log to the system logger.
> > > diff --git a/util.h b/util.h
> > > index 1adbf04..e8f99b6 100644
> > > --- a/util.h
> > > +++ b/util.h
> > > @@ -6,6 +6,14 @@
> > >  #ifndef UTIL_H
> > >  #define UTIL_H
> > >  
> > > +#define VERSION_BLOB							       \
> > > +	VERSION "\n"							       \
> > > +	"Copyright (C) 2020-2022 Red Hat\n"				       \  
> > 
> > Fwiw, I believe Red Hat legal suggests simply "Copyright Red Hat".
> > AIUI the "(C)" thing is legally meaningless (unlike the word
> > "copyright" or the actual © symbol), and the years are unnecessary and
> > tend to get out of date.
> 
> I just checked coreutils and gcc, but I see both points, I'll change
> this.

Yeah.. developer copyright statements have an awful lot of cargo culting.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]