[pacman-dev] [PATCH 4/4] Merge expac into src/pacman

[Andrew Gregory]

On 07/05/18 at 10:42am, Dave Reisner wrote:
> This introduces code which has long been maintained at:
> 
> https://github.com/falconindy/expac
> 
> From the README:
> 
> expac is a data extraction tool for alpm databases. It features
> printf-like flexibility and aims to be used as a simple tool for other
> pacman based utilities which don't link against the library. It uses
> pacman.conf as a config file for locating and loading your local and
> sync databases.
> ---
> I expect that I'll get some pushback on the use of #pragma here. I don't
> really know how portable it is to other compilers. Open to suggestions
> on how to better avoid this (though I'd prefer not to disable the
> warnings entirely for the file).

I'm not thrilled about adding #pragma to our codebase, but I don't see
a better solution.  I would think it would be at least as portable as
the function attributes we use elsewhere.

Just looking at expac.c, I don't see anything major, although I didn't
look all that hard because I assume expac is pretty well tested at
this point.  Since this is a previously external project, I'm fine if
you want to incorporate changes as further patches to keep this one as
close to original as possible.

>  doc/Makefile.am        |   7 +-
>  doc/expac.1.asciidoc   | 179 +++++++++
>  src/pacman/.gitignore  |   2 +
>  src/pacman/Makefile.am |   8 +-
>  src/pacman/expac.c     | 864 +++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 1057 insertions(+), 3 deletions(-)
>  create mode 100644 doc/expac.1.asciidoc
>  create mode 100644 src/pacman/expac.c

...

> diff --git a/src/pacman/expac.c b/src/pacman/expac.c
> new file mode 100644
> index 00000000..332414a0
> --- /dev/null
> +++ b/src/pacman/expac.c
> @@ -0,0 +1,864 @@

...

> +
> +typedef enum package_corpus_t {
> +	CORPUS_LOCAL,
> +	CORPUS_SYNC,
> +	CORPUS_FILE,
> +} package_corpus_t;

Is there a reason you don't use the PKG_FROM constants from alpm?

...

> +static int print_time(time_t timestamp) {
> +	char buffer[64];
> +	int out = 0;
> +
> +	if(!timestamp) {
> +		if(opt_verbose) {
> +			out += printf("None");
> +		}
> +		return out;
> +	}
> +
> +#pragma GCC diagnostic push
> +#pragma GCC diagnostic ignored "-Wformat-nonliteral"
> +	/* no overflow here, strftime prints a max of 64 including null */
> +	strftime(&buffer[0], 64, opt_timefmt, localtime(&timestamp));
> +#pragma GCC diagnostic pop
> +
> +	out += printf("%s", buffer);

I'm still trying to figure out how how the hell to correctly use
strftime with its multiple undefined behaviors and lack of an error
indicator, but we should at least skip the printf if it returns 0.  In
that case, either there is nothing to print or buffer contains
garbage.

...

> +static int read_targets_from_file(FILE *in, alpm_list_t **targets)
> +{
> +	char line[BUFSIZ];
> +	int i = 0, end = 0, targets_added = 0;
> +
> +	while(!end) {
> +		line[i] = fgetc(in);
> +
> +		if(feof(in))
> +			end = 1;
> +
> +		if(isspace(line[i]) || end) {

This should be line-delimited to match pacman rather than
space-delimited.

> +			line[i] = '\0';
> +			/* avoid adding zero length arg, if multiple spaces separate args */
> +			if(i > 0) {
> +				if(!alpm_list_find_str(*targets, line)) {
> +					*targets = alpm_list_add(*targets, strdup(line));
> +				}
> +				i = 0;
> +				++targets_added;
> +			}
> +		} else {
> +			++i;
> +			if(i >= BUFSIZ) {
> +				fprintf(stderr, "error: buffer overflow on stdin\n");
> +				return -1;
> +			}
> +		}
> +	}

...