Re: [Rpm-maint] [rpm-software-management/rpm] Add rpmlogOnce() and rpmlogReset() (PR #3417)
Merged #3417 into master. -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/3417#event-15100079278 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add rpmlogOnce() and rpmlogReset() (PR #3417)
@pmatilai commented on this pull request.
> @@ -22,6 +23,7 @@ struct rpmlogCtx_s {
unsigned mask;
int nrecsPri[RPMLOG_NPRIS];
std::vector recs;
+std::map, int>> seen;
Another note is that it's good to document such things in the commit message:
map worked out of the box, unordered_map didn't - had it been there, I wouldn't
have even brought this up.
--
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/3417#discussion_r1827363944
You are receiving this because you are subscribed to this thread.
Message ID: ___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add rpmlogOnce() and rpmlogReset() (PR #3417)
@pmatilai commented on this pull request.
> @@ -22,6 +23,7 @@ struct rpmlogCtx_s {
unsigned mask;
int nrecsPri[RPMLOG_NPRIS];
std::vector recs;
+std::map, int>> seen;
Oh, interesting. It didn't really even occur to me that you'd need to define a
hash for it when map didn't require any custom comparison stuff for this
structure. That's something to keep in mind.
Water gets deep very suddenly in C++ - on one line you're operating on
ridiculously high level and on the next one you run into an absurdly long
compiler error because you failed to define some low-level detail you'd kinda
expect the compiler to do for you because this is all standard datatypes.
Python it is not :sweat_smile:
--
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/3417#discussion_r1827359674
You are receiving this because you are subscribed to this thread.
Message ID: ___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add rpmlogOnce() and rpmlogReset() (PR #3417)
@ffesti commented on this pull request.
> @@ -22,6 +23,7 @@ struct rpmlogCtx_s {
unsigned mask;
int nrecsPri[RPMLOG_NPRIS];
std::vector recs;
+std::map, int>> seen;
Yeah, I thought about using an unordered map in the beginning and then opted to
go for the normal map just in case.
Good I did because I might not have been able to make the unordered_map work
right away. The missing hash function for pairs gives a pretty long compiler
message. But I got it working now.
--
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/3417#discussion_r1827335472
You are receiving this because you are subscribed to this thread.
Message ID: ___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add rpmlogOnce() and rpmlogReset() (PR #3417)
@ffesti pushed 2 commits. d0b48668e20fd1e60a70f770bd804a88d1a7fafd Add rpmlogOnce() and rpmlogReset() 6be14bd24eb29087c5d82b370a12988af4c057a2 Use rpmlogOnce() in handleHdrVS -- View it on GitHub: https://github.com/rpm-software-management/rpm/pull/3417/files/7b3377bb0c2bff99a06c5dd4d3aa7ef560549ccf..6be14bd24eb29087c5d82b370a12988af4c057a2 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add rpmlogOnce() and rpmlogReset() (PR #3417)
@ffesti commented on this pull request.
> @@ -412,3 +415,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) {
+ wrlock lock(ctx->mutex);
Technically yes. For now I don't want to over complicate things for what is
very little gains in practice.
--
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/3417#discussion_r1826531797
You are receiving this because you are subscribed to this thread.
Message ID: ___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add rpmlogOnce() and rpmlogReset() (PR #3417)
The added test looks alright, there was also a forgotten `.logDomain` initialization in `rpmReadPackageFile()`, apparently :sweat_smile:, now also fixed. I won't merge yet to give Florian a chance to act upon Panu's suggestions above (if he wishes so). Otherwise, it's good to go. -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/3417#issuecomment-2452260287 You are receiving this because you are subscribed to this thread. Message ID:___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add rpmlogOnce() and rpmlogReset() (PR #3417)
@pmatilai commented on this pull request.
> @@ -22,6 +23,7 @@ struct rpmlogCtx_s {
unsigned mask;
int nrecsPri[RPMLOG_NPRIS];
std::vector recs;
+std::map, int>> seen;
Something that occurred to me last evening after work: this would be better off
with unordered_map for both maps, which is STL's kind of exotic name for what
the industry knows as a hash map, std::map maintains things in sort order at
all times. Both work but unordered is (obviously) somewhat faster when you
don't need the sorted iteration etc.
Not that it'll matter much here because there'll be only a handful of items in
either map for the foreseeable future. So not a review requirement (although
changing is trivial because both maps dance to the same exact API), just ..
maybe education :sweat_smile:
--
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/3417#pullrequestreview-2409571315
You are receiving this because you are subscribed to this thread.
Message ID: ___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add rpmlogOnce() and rpmlogReset() (PR #3417)
@pmatilai commented on this pull request.
> @@ -412,3 +415,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) {
+ wrlock lock(ctx->mutex);
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.
--
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/3417#pullrequestreview-2409572741
You are receiving this because you are subscribed to this thread.
Message ID: ___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add rpmlogOnce() and rpmlogReset() (PR #3417)
recs is not really suited for this. It is a vector so lookups are not cheap. It only stores WARNING and above. We might want to link to the recs entry but even that does not really solve anything if the lower prio entries are missing. The additional string is there for cases where the message contains differing info. E.g. the package name or location of the error. Imagine suppressing a missing macro message. This will give a file and line number. And you do want that so you can look at the place where things go wrong in case it is just a typo. You still want to suppress the follow up messages if the macro is actually missing. So having a key allows to put them all under the same category. I don't think we will have logging objects for every possible occasion but only for majors things like builds or transactions. So I expect the domain to stay relevant even if we no longer have global logging. But that might just me being unimaginative. -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/3417#issuecomment-2449257525 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add rpmlogOnce() and rpmlogReset() (PR #3417)
@ffesti pushed 2 commits. f83a969b903a6cef614a2daa1e59650e1eeb0742 Add rpmlogOnce() and rpmlogReset() 7b3377bb0c2bff99a06c5dd4d3aa7ef560549ccf Use rpmlogOnce() in handleHdrVS -- View it on GitHub: https://github.com/rpm-software-management/rpm/pull/3417/files/02750bb69f9ad931fe26bef15a4b2615cdfcc27b..7b3377bb0c2bff99a06c5dd4d3aa7ef560549ccf You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add rpmlogOnce() and rpmlogReset() (PR #3417)
OK, test added. -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/3417#issuecomment-2450139574 You are receiving this because you are subscribed to this thread. Message ID:___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add rpmlogOnce() and rpmlogReset() (PR #3417)
@dmnks commented on this pull request.
> @@ -412,3 +415,43 @@ 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) {
+ wrlock lock(ctx->mutex);
+ newkey = !ctx->seen[domain][{code, key}]++;
No, it must be the other way around, hmm. :smile:
--
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/3417#discussion_r1824146939
You are receiving this because you are subscribed to this thread.
Message ID: ___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add rpmlogOnce() and rpmlogReset() (PR #3417)
@pmatilai commented on this pull request.
> @@ -412,3 +415,43 @@ 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) {
+ wrlock lock(ctx->mutex);
+ newkey = !ctx->seen[domain][{code, key}]++;
Maybe what it really needs is a high level comment stating what it does, and
leave the understanding as an exercise for the reader as per their C++
experience :sweat_smile:
--
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/3417#discussion_r1824170519
You are receiving this because you are subscribed to this thread.
Message ID: ___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add rpmlogOnce() and rpmlogReset() (PR #3417)
OK; this should address all issues except for the Python based test case which I will add next. -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/3417#issuecomment-2449444799 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add rpmlogOnce() and rpmlogReset() (PR #3417)
@ffesti pushed 2 commits. c0bb2aaf51f78ac30e86e65f772c17d4c8dde8ae Add rpmlogOnce() and rpmlogReset() 02750bb69f9ad931fe26bef15a4b2615cdfcc27b Use rpmlogOnce() in handleHdrVS -- View it on GitHub: https://github.com/rpm-software-management/rpm/pull/3417/files/bf6baaa3ef7d62959064583d5ead2d64a18fda0c..02750bb69f9ad931fe26bef15a4b2615cdfcc27b You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add rpmlogOnce() and rpmlogReset() (PR #3417)
@dmnks commented on this pull request.
> @@ -412,3 +415,43 @@ 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) {
+ wrlock lock(ctx->mutex);
+ newkey = !ctx->seen[domain][{code, key}]++;
Actually now that I think about it, this follows from the definition of the ++
operator.
--
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/3417#discussion_r1824148651
You are receiving this because you are subscribed to this thread.
Message ID: ___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add rpmlogOnce() and rpmlogReset() (PR #3417)
@dmnks commented on this pull request.
> @@ -412,3 +415,43 @@ 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) {
+ wrlock lock(ctx->mutex);
+ newkey = !ctx->seen[domain][{code, key}]++;
Just proves Panu's point of this being perhaps a bit confusing :smile:
--
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/3417#discussion_r1824147547
You are receiving this because you are subscribed to this thread.
Message ID: ___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add rpmlogOnce() and rpmlogReset() (PR #3417)
@dmnks commented on this pull request.
> @@ -412,3 +415,43 @@ 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) {
+ wrlock lock(ctx->mutex);
+ newkey = !ctx->seen[domain][{code, key}]++;
Hmm, actually... it should be the opposite, otherwise it wouldn't work as
intended.
--
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/3417#discussion_r1824138164
You are receiving this because you are subscribed to this thread.
Message ID: ___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add rpmlogOnce() and rpmlogReset() (PR #3417)
@ffesti commented on this pull request.
> + }
+ va_end(ap);
+}
+errno = saved_errno;
+return newkey;
+}
+
+void rpmlogReset(uint64_t domain, int mode=0)
+{
+rpmlogCtx ctx = rpmlogCtxAcquire();
+std::map, int> domain_data = {};
+
+if (ctx) {
+ wrlock lock(ctx->mutex);
+ if (mode)
+ domain_data = ctx->seen[domain];
Deleted.
--
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/3417#discussion_r1824115788
You are receiving this because you are subscribed to this thread.
Message ID: ___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add rpmlogOnce() and rpmlogReset() (PR #3417)
@dmnks commented on this pull request.
> @@ -412,3 +415,43 @@ 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) {
+ wrlock lock(ctx->mutex);
+ newkey = !ctx->seen[domain][{code, key}]++;
For me, the most confusing part is the increment vs. negation, it's not clear
which one takes precedence (without looking it up in the C++ docs). I assume
it's first incremented and only then negated, right?
--
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/3417#discussion_r1824137367
You are receiving this because you are subscribed to this thread.
Message ID: ___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add rpmlogOnce() and rpmlogReset() (PR #3417)
I don't disagree that this can probably be done better with proper OO. But I don't want to get into this right now. Redoing the whole rpmlog thing is a story for another time. As this is a internal API for now we can still change it later one when we get to that. -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/3417#issuecomment-2449381162 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add rpmlogOnce() and rpmlogReset() (PR #3417)
@pmatilai commented on this pull request.
> @@ -412,3 +415,43 @@ 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) {
+ wrlock lock(ctx->mutex);
+ newkey = !ctx->seen[domain][{code, key}]++;
"This" is unclear :sweat_smile: Maybe something to the tune of "[] access on
map creates the element on first access", but the more you think about it the
more the comment starts seeming redundant :sweat_smile:
So dunno - up to you.
--
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/3417#discussion_r1824130401
You are receiving this because you are subscribed to this thread.
Message ID: ___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add rpmlogOnce() and rpmlogReset() (PR #3417)
> Redoing the whole rpmlog thing is a story for another time. [...] > but we really cannot get into that business now. It'll need to be thought > through once we decide to do a logging overhaul. Agreed, all are good points. Let's go ahead with this. -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/3417#issuecomment-2449393522 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add rpmlogOnce() and rpmlogReset() (PR #3417)
And yup, for reasons like this it's best done as an internal-only API just now - so we don't need to commit to it for the rest of our lives :sweat_smile: -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/3417#issuecomment-2449386312 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add rpmlogOnce() and rpmlogReset() (PR #3417)
Yeah the domain and its attached seen-stuff could be tracked as a proper object too, I mumbled something along those lines in #3389 actually, the id is just a kind of simplified version of that. The log context itself is still a kind of different thing because at least in rpm, if you specify a transaction log context, then what other output is there during a transaction? Ditto for a package build? And they'd all probably be outputing to the same stderr/file anyhow. Maybe there's a case, it's entirely possible, but we really cannot get into *that* business now. It'll need to be thought through once we decide to do a logging overhaul. -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/3417#issuecomment-2449383891 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add rpmlogOnce() and rpmlogReset() (PR #3417)
@ffesti commented on this pull request.
> @@ -412,3 +415,43 @@ 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) {
+ wrlock lock(ctx->mutex);
+ newkey = !ctx->seen[domain][{code, key}]++;
Yeah this needs a comment. Is `/* This gets initialized automatically on first
access */` clear enough?
--
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/3417#discussion_r1824114894
You are receiving this because you are subscribed to this thread.
Message ID: ___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add rpmlogOnce() and rpmlogReset() (PR #3417)
@pmatilai commented on this pull request.
> @@ -412,3 +415,43 @@ 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) {
+ wrlock lock(ctx->mutex);
+ newkey = !ctx->seen[domain][{code, key}]++;
Mind you, I don't find this unreadable, just needing a bit of thought too. I
guess the thing that raises *my* alarms on first sight is the unchecked []
access which is fine with a map but undefined behavior in some others.
Considering we're all relative C++ beginners here, some such remark would not
be out of line.
--
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/3417#discussion_r1824104313
You are receiving this because you are subscribed to this thread.
Message ID: ___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add rpmlogOnce() and rpmlogReset() (PR #3417)
@pmatilai commented on this pull request.
> @@ -412,3 +415,43 @@ 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) {
+ wrlock lock(ctx->mutex);
+ newkey = !ctx->seen[domain][{code, key}]++;
Yeah there's a lot of magic in that line, perhaps a bit too much. This isn't a
LoC competition, understandability is 100x more important. If @ffesti himself
needed to think for a while to convince himself why this works, that's a danger
sign already, maybe this could be opened up a bit, or at least add a comment.
--
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/3417#discussion_r1824094315
You are receiving this because you are subscribed to this thread.
Message ID: ___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add rpmlogOnce() and rpmlogReset() (PR #3417)
@pmatilai commented on this pull request.
> @@ -412,3 +415,43 @@ 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) {
But a cleaner version is actually to wrap the seen-check and the lock into a
small seenLog() style helper function.
--
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/3417#discussion_r1824047197
You are receiving this because you are subscribed to this thread.
Message ID: ___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add rpmlogOnce() and rpmlogReset() (PR #3417)
> Yeah, logging is kind of global in nature, I don't see that fundamentally > changing. The deal with _allowing_ non-global context is to allow something > like dnf to have a context _they_ control, not so much with having multiple > different contexts active within rpm at the same time. So I don't see this > logonce and domain overlapping with that stuff much, if at all really. It just seems to me like this domain tracking (based on an integer key) tries to emulate proper OOP encapsulation. IOW, if an object needs a log "context", it might as well get one, represented as an actual object. The users wanting to emit messages for that context would then pass the object to the logging functions, much like we now pass the domain integer. -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/3417#issuecomment-2449347101 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add rpmlogOnce() and rpmlogReset() (PR #3417)
> recs is not really suited for this. It is a vector so lookups are not cheap. > It only stores WARNING and above. We might want to link to the recs entry but > even that does not really solve anything if the lower prio entries are > missing. Sure, the logic for populating `recs` would have to be adjusted, my point is that message de-duplication can also be thought of as filtering, and we already do by-mask filtering in `rpmlogPrintByMask()`. What I'm getting at is that we could just record all the messages and only decide how to collapse (de-duplicate) them into a single meaningful message after the fact, perhaps even using the information in those messages somehow. Yes, we need to do that on-the-fly as well, of course, but that would just be a special case of that. That said, I haven't thought this through too much, it's just my gut feeling speaking :smile: Having this specialized `seen` map also works and we can always refactor it in the future, it's not a big deal. > The additional string is there for cases where the message contains differing > info. E.g. the package name or location of the error. Imagine suppressing a > missing macro message. This will give a file and line number. And you do want > that so you can look at the place where things go wrong in case it is just a > typo. You still want to suppress the follow up messages if the macro is > actually missing. So having a key allows to put them all under the same > category. Indeed. I had this in the back of my mind too, just needed to see it written down for it to click. Thanks! > I don't think we will have logging objects for every possible occasion but > only for majors things like builds or transactions. So I expect the domain to > stay relevant even if we no longer have global logging. But that might just > me being unimaginative. Right, it's just that "context" coincides with "domain" in a way, but we already use the term "context" for a different purpose in the logging module, so yep. -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/3417#issuecomment-2449336976 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add rpmlogOnce() and rpmlogReset() (PR #3417)
@pmatilai commented on this pull request.
> @@ -412,3 +415,43 @@ 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) {
Don't add a bogus check just to create a scope, if you need a scope then just
use a plain { } block.
--
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/3417#discussion_r1824045133
You are receiving this because you are subscribed to this thread.
Message ID: ___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add rpmlogOnce() and rpmlogReset() (PR #3417)
Also, this is two entirely different things in a single commit. Split them up. -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/3417#issuecomment-2449321967 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add rpmlogOnce() and rpmlogReset() (PR #3417)
> I don't think we will have logging objects for every possible occasion but > only for majors things like builds or transactions. So I expect the domain to > stay relevant even if we no longer have global logging. Yeah, logging is kind of global in nature, I don't see that fundamentally changing. The deal with *allowing* non-global context is to allow something like dnf to have a context *they* control, not so much with having multiple different contexts active within rpm at the same time. So I don't see this logonce and domain overlapping with that stuff much, if at all really. -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/3417#issuecomment-2449308014 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add rpmlogOnce() and rpmlogReset() (PR #3417)
The existing tests don't cover #3336 which this claims to be fixing. Rpm wont do multiple transactions in a process, but you should be able to test it with the python bindings. -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/3417#issuecomment-2449318456 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add rpmlogOnce() and rpmlogReset() (PR #3417)
@ffesti pushed 1 commit. bf6baaa3ef7d62959064583d5ead2d64a18fda0c Add rpmlogOnce() and rpmlogReset() -- View it on GitHub: https://github.com/rpm-software-management/rpm/pull/3417/files/7eb7b473a04ccad9457bbd0de1bc6c7f52d11b93..bf6baaa3ef7d62959064583d5ead2d64a18fda0c You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add rpmlogOnce() and rpmlogReset() (PR #3417)
@dmnks commented on this pull request.
> @@ -412,3 +415,43 @@ 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) {
> 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.
--
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/3417#discussion_r1824064306
You are receiving this because you are subscribed to this thread.
Message ID: ___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add rpmlogOnce() and rpmlogReset() (PR #3417)
@pmatilai commented on this pull request.
> + }
+ va_end(ap);
+}
+errno = saved_errno;
+return newkey;
+}
+
+void rpmlogReset(uint64_t domain, int mode=0)
+{
+rpmlogCtx ctx = rpmlogCtxAcquire();
+std::map, int> domain_data = {};
+
+if (ctx) {
+ wrlock lock(ctx->mutex);
+ if (mode)
+ domain_data = ctx->seen[domain];
Don't add speculative dead code, this only looks baffling. Add handling for
other modes when you add them.
--
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/3417#pullrequestreview-2407327566
You are receiving this because you are subscribed to this thread.
Message ID: ___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add rpmlogOnce() and rpmlogReset() (PR #3417)
@ffesti commented on this pull request.
> @@ -412,3 +415,43 @@ 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) {
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.
--
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/3417#discussion_r1824017885
You are receiving this because you are subscribed to this thread.
Message ID: ___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add rpmlogOnce() and rpmlogReset() (PR #3417)
@dmnks commented on this pull request.
A couple of questions:
1. We already know what messages (code & string) have been emitted in the log
context as they're stored in the `recs` member (currently a vector). Is there
any benefit of adding a new `seen` map, instead of turning `recs` into such a
map, adding a counter to `rpmlogRec_s` and using that?
2. Is there any use case for keying on an additional string rather than on the
actual log message?
3. Once we support non-global logging contexts, the `domain` would no longer be
needed. That is, all users/domains (e.g. `ts`) would get a separate log
context, instead of tracking them centrally in the global context. Is that
correct?
I see that this is meant to be a PoC kind of thing, and as such it looks fine
(some comments inline).
@pmatilai, any other opinion?
> @@ -412,3 +415,43 @@ 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) {
+ wrlock lock(ctx->mutex);
+ newkey = !ctx->seen[domain][{code, key}]++;
Showcases the power of C++, nicely done! :smile:
> @@ -412,3 +415,43 @@ 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) {
Is this check necessary? We don't seem to be doing it elsewhere in this file
(the global `ctx` is always returned).
--
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/3417#pullrequestreview-2405628199
You are receiving this because you are subscribed to this thread.
Message ID: ___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
