[PATCH Try#2 3/3] Major improvements to RPM

[Denis Vlasenko]

On Saturday 09 June 2007 12:57, andy at warmcat.com wrote:
> Adds many new features to RPM including a 'database' of installed
> packages down /var/lib/rpm.  See the comments in rpm.c and the
> usage help for more information.
> 
> Signed-off-by: Andy Green <andy at warmcat.com>

Wow, this is BIG! Thanks.

Will probably need several iterations of review.

> +#include <dirent.h>
> +#include <sys/types.h> /* For chmod */
> +#include <sys/stat.h> /* For chmod */
> +#include <sys/wait.h> /* For waitpid() */
>  #include "libbb.h"

libbb.h already includes dirent.h and sys/*.

"rpm.h" is used only in rpm.c - why do you need it at all?


> +static const char * szaRelationship[] = {
> +	"", "<", ">", "", "=", "<=", ">=", ""
>  };

Vector of CONST ptrs to const char:

const char *const szaRelationship[]


> +int
> +rpm_compare_versions(const char * sz1, const char * sz2)

should be "static int ...".

> +		if ((prss->m_offset = lseek(prss->m_fd, 0, SEEK_CUR)) ==
> +		    (off_t)-1)
> +			perror("Seek error B");

Think how likely each particular seek is to fail. If it is not
likely to do it, just do xlseek(). In rarer cases where failure
is more or less expected AND you know what program should do
if seek failed (instead of errmsg+exit like xlseek does),
only then do as above (but use bb_perror_msg).

Currently in many places code seems to perror() and ... continue
like there was no error. This doesn't seem right.

(And please dont do "if ((a = f()) == b)", do "a = f(); if (a == b)")

> +		if (!prss->m_tags) {
> +			printf("Error reading rpm header\n");
> +			exit(-1);
> +		}

bb_perror_msg_and_die("error reading rpm header")

> +		int fd = open(szFilepathTemp, O_CREAT|O_TRUNC|O_WRONLY, 0700);

need to error check. xopen?

> +		write(fd, sz, strlen(sz));

xwrite?

> -		xread(fd, &header, sizeof(header));
> -		if (strncmp((char *) &header.magic, RPM_HEADER_MAGIC, 3) != 0)
> +		read(fd, &header, sizeof(header));
> +		if (strncmp((char *) &header.magic, RPM_HEADER_MAGIC, 3))
>  			return NULL; /* Invalid magic */

*removing* error checking xfunc? why?

> +				bb_error_msg_and_die(
> +				    "Too many packages in transaction set");

bb_[p]error[_and_die]() message should start with small letter ("too many...").

> +	cwd = xmalloc(PATH_MAX);
> +	getcwd(cwd, PATH_MAX);

We have xmalloc_getcwd_or_die() which eats much less mem.

> +	while ((opt = getopt(argc, argv, "iqpldcr:eofVUaFC:")) != -1) {

maybe getopt32?

> +			if (nFuncEffective & rpm_package) {
> +
> +				/* operating on a given RPM */

Indented far too much to the right.
Your function gets too big.
Split into smaller functions if possible.


> +void
> +fileaction_verify(char *filename, int fileref, rpm_scan_state * pstate)
> +{
> +
> +	switch (rpm_compare_md5_string_to_actual(filename,
> +	    rpm_getstring(RPMTAG_FILEMD5S, fileref, pstate))) {
> +
> +		case 1:
> +			printf(" CHANGED %s\n", filename);
> +			pstate->m_nUser++;
> +			break;
> +		case 2:
> +		case 3:
> +		case 4:
> +			printf(" DELETED %s\n", filename);
> +			pstate->m_nUser++;
> +			break;
> +
> +		default:
> +			break;
>  	}
> +
>  }

There are many blank lines in the source, like above. ?


> +enum rpm_functions_e {
> +	rpm_query = 1,
> +	rpm_install = 2,
> +	rpm_query_info = 4,
> +	rpm_package = 8,
> +	rpm_query_list = 16,

Make it more typo-resistant by ... = 1 << 4

> +	rpm_query_list_doc = 32,

... = 1 << 5

> +	rpm_query_list_config = 64,
> +	rpm_update = 128,
> +	rpm_erase = 256,
> +	rpm_oldpackage = 512,
> +	rpm_force = 1024,

... = 1 << 10 etc

> +	rpm_query_verify = 2048,
> +	rpm_query_all = 4096,
> +	rpm_skip_uninstalled = 8192,
> +	rpm_query_compareverions = 16384


--
vda