Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add rpmlogOnce() and rpmlogReset() #3417

Merged
merged 2 commits into from
Nov 4, 2024
Merged

Add rpmlogOnce() and rpmlogReset() #3417

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d0b4866
Add rpmlogOnce() and rpmlogReset()
ffesti Sep 27, 2024
6be14bd
Use rpmlogOnce() in handleHdrVS
ffesti Sep 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 8 additions & 24 deletions lib/package.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

#include "rpmlead.hh"
#include "rpmio_internal.hh" /* fd digest bits */
#include "rpmlog_internal.hh" /* rpmlogOnce */
#include "header_internal.hh" /* XXX headerCheck */
#include "rpmvs.hh"

Expand All @@ -31,6 +32,7 @@ struct pkgdata_s {
hdrvsmsg msgfunc;
const char *fn;
char *msg;
uint64_t logDomain;
rpmRC rc;
};

Expand Down Expand Up @@ -110,28 +112,6 @@ rpmTagVal headerMergeLegacySigs(Header h, Header sigh, char **msg)
return xl->stag;
}

/**
* Remember current key id.
* XXX: This s*** needs to die. Hook it into keyring or sumthin...
* @param keyid signature keyid
* @return 0 if new keyid, otherwise 1
*/
static int stashKeyid(const char *keyid)
{
static std::mutex keyid_mutex;
static std::set<std::string> keyids;
int seen = 0;

if (keyid == NULL)
return 0;

std::lock_guard<std::mutex> lock(keyid_mutex);
auto ret = keyids.insert(keyid);
seen = (ret.second == false);

return seen;
}

static int handleHdrVS(struct rpmsinfo_s *sinfo, void *cbdata)
{
struct pkgdata_s *pkgdata = (struct pkgdata_s *)cbdata;
Expand Down Expand Up @@ -170,6 +150,7 @@ rpmRC headerCheck(rpmts ts, const void * uh, size_t uc, char ** msg)
.msgfunc = appendhdrmsg,
.fn = NULL,
.msg = NULL,
.logDomain = (uint64_t) ts,
.rc = RPMRC_OK,
};

Expand Down Expand Up @@ -294,8 +275,8 @@ static void loghdrmsg(struct rpmsinfo_s *sinfo, struct pkgdata_s *pkgdata,
case RPMRC_NOTTRUSTED: /* Signature is OK, but key is not trusted. */
case RPMRC_NOKEY: /* Public key is unavailable. */
/* XXX Print NOKEY/NOTTRUSTED warning only once. */
if (stashKeyid(sinfo->keyid) == 0)
lvl = RPMLOG_WARNING;
if (rpmlogOnce(pkgdata->logDomain, sinfo->keyid, RPMLOG_WARNING, "%s: %s\n", pkgdata->fn, msg))
goto exit;
break;
case RPMRC_NOTFOUND: /* Signature/digest not present. */
lvl = RPMLOG_WARNING;
Expand All @@ -307,6 +288,8 @@ static void loghdrmsg(struct rpmsinfo_s *sinfo, struct pkgdata_s *pkgdata,
}

rpmlog(lvl, "%s: %s\n", pkgdata->fn, msg);
exit:
;
}

rpmRC rpmReadPackageFile(rpmts ts, FD_t fd, const char * fn, Header * hdrp)
Expand All @@ -323,6 +306,7 @@ rpmRC rpmReadPackageFile(rpmts ts, FD_t fd, const char * fn, Header * hdrp)
.msgfunc = loghdrmsg,
.fn = fn ? fn : Fdescr(fd),
.msg = NULL,
.logDomain = (uint64_t) ts,
.rc = RPMRC_OK,
};

Expand Down
2 changes: 2 additions & 0 deletions lib/rpmts.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
#include "rpmplugins.hh"
#include "rpmts_internal.hh"
#include "rpmte_internal.hh"
#include "rpmlog_internal.hh"
#include "misc.hh"
#include "rpmtriggers.hh"

Expand Down Expand Up @@ -575,6 +576,7 @@ rpmts rpmtsFree(rpmts ts)
ts->plugins = rpmpluginsFree(ts->plugins);

rpmtriggersFree(ts->trigs2run);
rpmlogReset((uint64_t) ts);

if (_rpmts_stats)
rpmtsPrintStats(ts);
Expand Down
48 changes: 47 additions & 1 deletion rpmio/rpmlog.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include <mutex>
#include <shared_mutex>
#include <vector>
#include <unordered_map>
#include <string>

#include <stdarg.h>
Expand All @@ -17,11 +18,22 @@
#include <rpm/rpmstring.h>
#include "debug.h"

struct pair_hash {
template <class T1, class T2>
std::size_t operator () (const std::pair<T1,T2> &p) const {
auto h1 = std::hash<T1>{}(p.first);
auto h2 = std::hash<T2>{}(p.second);
/* based on boost::hash_combine */
return h2 + 0x9e3779b9 + (h1 << 6) + (h1 >> 2);
}
};

typedef struct rpmlogCtx_s * rpmlogCtx;
struct rpmlogCtx_s {
unsigned mask;
int nrecsPri[RPMLOG_NPRIS];
std::vector<rpmlogRec_s> recs;
std::unordered_map<uint64_t, std::unordered_map<std::pair<int, std::string>, int, pair_hash>> seen;
rpmlogCallback cbfunc;
rpmlogCallbackData cbdata;
FILE *stdlog;
Expand All @@ -41,7 +53,7 @@ using rdlock = std::shared_lock<std::shared_mutex>;
static rpmlogCtx rpmlogCtxAcquire()
{
static struct rpmlogCtx_s _globalCtx = { RPMLOG_UPTO(RPMLOG_NOTICE),
{0}, {}, NULL, NULL, NULL };
{0}, {}, {}, NULL, NULL, NULL };
return &_globalCtx;
}

Expand Down Expand Up @@ -127,6 +139,7 @@ void rpmlogClose (void)
wrlock lock(ctx->mutex);

ctx->recs.clear();
ctx->seen.clear();
memset(ctx->nrecsPri, 0, sizeof(ctx->nrecsPri));
}

Expand Down Expand Up @@ -412,3 +425,36 @@ void rpmlog (int code, const char *fmt, ...)
exit:
errno = saved_errno;
}

int rpmlogOnce (uint64_t domain, const char * key, int code, const char *fmt, ...)
{
int saved_errno = errno;
rpmlogCtx ctx = rpmlogCtxAcquire();
int newkey = 0;

if (ctx) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this check necessary? We don't seem to be doing it elsewhere in this file (the global ctx is always returned).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not. It is there to limit the scope of the lock. If you want to use rpmlock below to print the numbers for the repeated messages we can't hold a lock there.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't add a bogus check just to create a scope, if you need a scope then just use a plain { } block.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But a cleaner version is actually to wrap the seen-check and the lock into a small seenLog() style helper function.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not. It is there to limit the scope of the lock. If you want to use rpmlock below to print the numbers for the repeated messages we can't hold a lock there.

Oh right, that didn't occur to me, thanks for clarifying.

wrlock lock(ctx->mutex);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another thing that occurred to me last evening is that technically this should use a mutex of its own - there's no reason to prevent other logging events from proceeding while we filter out other messages. Not that it matters here so just a random remark, not a requirement.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically yes. For now I don't want to over complicate things for what is very little gains in practice.

/* members get initialized automatically on first access */
newkey = !(ctx->seen[domain][{code, key}]++);
}

if (newkey) {
va_list ap;
char *msg = NULL;
va_start(ap, fmt);
if (rvasprintf(&msg, fmt, ap) >= 0) {
rpmlog(code, msg);
free(msg);
}
va_end(ap);
}
errno = saved_errno;
return newkey;
}

void rpmlogReset(uint64_t domain, int mode=0)
{
rpmlogCtx ctx = rpmlogCtxAcquire();
wrlock lock(ctx->mutex);
ctx->seen.erase(domain);
}
23 changes: 23 additions & 0 deletions rpmio/rpmlog_internal.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
#ifndef H_RPMLOG_INTERNAL
#define H_RPMLOG_INTERNAL 1


/** \ingroup rpmlog
* Generate a log message using FMT string and option arguments.
* Only actually log on the first time passing the key value
* @param domain group of messages to be reset together
* @param key key to match log messages together
* @param code rpmlogLvl
* @param fmt format string and parameter to render
* @return 1 if actually logging 0 otherwise
*/
int rpmlogOnce (uint64_t domain, const char * key, int code, const char *fmt, ...) RPM_GNUC_PRINTF(4, 5);

/** \ingroup rpmlog
* Clear memory of logmessages for a given domain
* @param domain group of messages to be reset together
* @param mode curretnly only 0 supported whihc drops everything
*/
void rpmlogReset(uint64_t domain, int mode=0);

#endif
18 changes: 18 additions & 0 deletions tests/rpmpython.at
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -353,6 +353,24 @@ for e in ts:
lock]
)

RPMPY_TEST([log suppression per transaction],[
for i in range(4):
try:
ts.addInstall('${RPMDATA}/RPMS/hello-2.0-1.x86_64-signed-with-subkey.rpm', 'u')
except rpm.error as e:
print(e)
if i==1:
ts = rpm.ts()
],
[public key not available
public key not available
public key not available
public key not available
],
[warning: /data/RPMS/hello-2.0-1.x86_64-signed-with-subkey.rpm: Header V4 RSA/SHA512 Signature, key ID 1f71177215217ee0: NOKEY
warning: /data/RPMS/hello-2.0-1.x86_64-signed-with-subkey.rpm: Header V4 RSA/SHA512 Signature, key ID 1f71177215217ee0: NOKEY]
)

RPMPY_TEST([transaction callback 1],[
def ocb(what, amount, total, key, data):
print(what, amount, total, type(key), data)
Expand Down
Loading