Re: [PATCH 2/6] xenconsoled: switch hypervisor log to use logfile abstraction

[Ian Jackson <ian.jackson@eu.citrix.com>]

Wei Liu writes ("[PATCH 2/6] xenconsoled: switch hypervisor log to use logfile abstraction"):
> To minimise code churn, copy and paste a some existing functions and
> adapt them to write to logfile. The functions to deal with fd directly
> will go away eventually.
...
> +static int write_logfile(struct logfile *logfile, const char *buf,
> +			 size_t len)
> +{
> +	ssize_t r = logfile_append(logfile, buf, len);
> +
> +	if (r < 0) return -1;
> +	return 0;
> +}

That this is necessary suggests that logfile_append has an
inconvenient calling convention.

> +static int write_logfile_with_timestamp(struct logfile *logfile,
> +					const char *data, size_t sz,
> +					int *needts)
> +{

This should be in logfile.c.  Furthermore, this is at the wrong level
with respect to log rotation:

> +		if ((*needts && logfile_append(logfile, ts, tslen))

This could result in a timestamp being split across two different
logfiles which is undesirable.  So I think this needs to be done at
the layer below.

> +		data = nl + 1;
> +		if (found_nl) {
> +			// If we printed a newline, strip all \r following it
> +			while (data <= last_byte && *data == '\r')
> +				data++;
> +		}

I would prefer that we only apply a lossless transformation to
logfiles, even if that means that our lotfiles contain CRs.  Also,
this is rather odd.  Surely it is more usual for the CR to precede the
LF.

I suggest the transformation `after every LF, preface the next byte
with the timestamp of that next byte'.

> -	if (fd != -1 && log_time_hv) {
> -		if (write_with_timestamp(fd, "Logfile Opened\n",
> -					 strlen("Logfile Opened\n"),
> -					 &log_time_hv_needts) < 0) {
> +
> +	if (tmp && log_time_hv) {
> +		if (write_logfile_with_timestamp(tmp, "Logfile Opened\n",
> +						 strlen("Logfile Opened\n"),
> +						 &log_time_hv_needts) < 0) {

I think that if timestamps are turned off, writing the "Logfile
Opened" (with no preceding timestamp) is probably a good idea.  So
this can be made simpler.

>  	if (log_hv) {
> -		if (log_hv_fd != -1)
> -			close(log_hv_fd);
> -		log_hv_fd = create_hv_log();
> +		if (log_hv_file)
> +			logfile_free(log_hv_file);
> +		log_hv_file = create_hv_log();

You could separate out the nonfunctional changes if you did it in
stages:

 * provide logfile_valid as a dummy macro that tests against -1

 * replace `log_hv_fd' with `log_hv' throughout; replace
   comparisons with -1 with logfile_valid(log_hv); likewise
   client logfiles - no functional change

 * replace log opening code with calls to logfile_open; change
   types; change implementation of logfile_valid

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel