[Rpm-maint] [PATCH 3/3] Add RPMTAG_IDENTITY calculation as tag extension

[Panu Matilainen]

On 8/23/18 12:01 AM, Vladimir D. Seleznev wrote:
> RPMTAG_IDENTITY is calculating as digest of part of package header that
> marked for identity calculation in rpmtag.h. They are supposed to not
> contain irrelevant to package build tag entries.
> 
> Mathematically RPMTAG_IDENTITY value is a result of function of two
> variable: a package header and an rpm utility, thus this value can
> differ for same package and different version of rpm.
> 
> Signed-off-by: Vladimir D. Seleznev <vseleznv at altlinux.org>
> ---
>   lib/tagexts.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 72 insertions(+)
> 
> diff --git a/lib/tagexts.c b/lib/tagexts.c
> index 61e9b702b..1467bfcd3 100644
> --- a/lib/tagexts.c
> +++ b/lib/tagexts.c
> @@ -954,6 +954,77 @@ static int filenlinksTag(Header h, rpmtd td, headerGetFlags hgflags)
>   
>   #include "identitymatch.C"
>   
> +static int identityTag(Header h, rpmtd td, headerGetFlags hgflags)
> +{
> +    int rc = 1;
> +    DIGEST_CTX ctx;
> +    char * identity = NULL;
> +
> +    struct rpmtd_s ttd;
> +    Header ih;
> +    HeaderIterator hi;
> +
> +    /* Return immediatly if there is no identity marker in rpmtag.h */
> +    if (!identityMatch(0)) {
> +	rc = 0;
> +	goto exit;
> +    }
> +
> +    /* rpm v3 packages dont have an immutable region */
> +    if (!headerGet(h, RPMTAG_HEADERIMMUTABLE, &ttd, HEADERGET_DEFAULT)) {
> +	rc = 0;
> +	goto exit;
> +    }
> +
> +    ih = headerImport(ttd.data, ttd.count, HEADERIMPORT_FAST);
> +    if ((hi = headerInitIterator(ih)) == NULL) {
> +	rc = 0;
> +	goto exit;
> +    }
> +
> +    if ((ctx = rpmDigestInit(PGPHASHALGO_SHA256, RPMDIGEST_NONE)) == NULL) {
> +	rc = 0;

That's no less than four cases setting rc to failure before jumping out. 
I'd suggest expecting failure to begin with, and only setting it to 
success one at the end. Which is a common idiom all over the codebase.

> +	goto exit;
> +    }
> +
> +    while (headerNext(hi, &ttd) && rc) {
> +	if (rpmtdCount(&ttd) > 0) {

There should be no need for this, tags with zero count don't exist in 
headers AFAIK.

> +	    if (identityMatch(ttd.tag)) {
> +		int i;
> +		rpmtdInit(&ttd);

No need for rpmtdInit() here either, headerNext() takes care of that.

> +		while ((i = rpmtdNext(&ttd)) >= 0) {
> +		    rpmDigestUpdate(ctx, &ttd.tag, sizeof(ttd.tag));
> +		    rpmDigestUpdate(ctx, &i, sizeof(i));
> +		    if (ttd.type == RPM_STRING_ARRAY_TYPE) {
> +			for (int j = 0; j < ttd.count; j++)
> +			    rpmDigestUpdate(ctx, ((char **) ttd.data)[j], strlen(((char **) ttd.data)[j]));
> +			continue;
> +		    }
> +		    rpmDigestUpdate(ctx, ttd.data, ttd.size);

I tend to agree with Jeff on this one - everything can be converted to 
strings after which all datatypes can be handled identically, and more 
simply.

> +		}
> +	    }
> +	}
> +	rpmtdFreeData(&ttd);
> +    }
> +
> +    if (!rc)
> +	goto exit;
> +
> +    rpmDigestFinal(ctx, (void **) &identity, NULL, 1);
> +    headerFree(ih);
> +
> +    td->data = xstrdup(identity);
> +    td->type = RPM_STRING_TYPE;
> +    td->count = 1;
> +    td->flags = RPMTD_ALLOCED;
> +
> +exit:
> +    headerFreeIterator(hi);
> +    _free(identity);

Why strdup() of already malloced data? Just pass address of td->data 
directly to rpmDigestFinal() and avoid this unnecessary dance.

	- Panu -

> +
> +    return rc;
> +}
> +
>   static const struct headerTagFunc_s rpmHeaderTagExtensions[] = {
>       { RPMTAG_GROUP,		groupTag },
>       { RPMTAG_DESCRIPTION,	descriptionTag },
> @@ -992,6 +1063,7 @@ static const struct headerTagFunc_s rpmHeaderTagExtensions[] = {
>       { RPMTAG_OBSOLETENEVRS,	obsoletenevrsTag },
>       { RPMTAG_CONFLICTNEVRS,	conflictnevrsTag },
>       { RPMTAG_FILENLINKS,	filenlinksTag },
> +    { RPMTAG_IDENTITY,		identityTag },
>       { 0, 			NULL }
>   };
>   
>