This seems like a huge overkill when in 4.19.1 there's exactly one
rpmfiStat() call that unnecessarily invokes an rpmug lookup in a threaded
scenario, but then rpmfiStat() and rpmfilesStat() are public APIs that people
expect to be safe for use within the originating thread.
Collect all the cac
(commit message tweaked somewhat in the later pushes)
--
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/2843#issuecomment-1891985861
You are receiving this because you are subscribed to this thread.
Message ID: _
@Conan-Kudo approved this pull request.
--
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/2843#pullrequestreview-1821554619
You are receiving this because you are subscribed to this thread.
Message ID: __
I wonder if this leaks memory. We create a new struct for each thread. Yes,
this is freed in rpmChrootSet but that is relying a lot on the right call
order.
--
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/2843#issuecomment-1893419277
Yo
It does leak, should've mentioned that in the commit message. In the sense that
each thread calling it will leave one cache around and reachable. It's far from
ideal but it's basically an emergency bandaid.
What do you mean about ending itself?
--
Reply to this email directly or view it on Git
OK, "ending itself" is a bit over dramatic... rpmChrootSet instantly returns if
there is now chroot so rpmugFree() is never called at all.
--
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/2843#issuecomment-1893557584
You are receiving this
Yup, but this patch doesn't change that, the "leak" is already there. This only
makes it per-thread.
--
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/2843#issuecomment-1893568039
You are receiving this because you are subscribed to this th
It wouldn't be hard to introduce per-thread locking instead. The bigger deal
here is the new central struct that makes it possible to do stuff, perhaps I
should actually split that into a separate commit and then consider the thread
safety separately. The reason its lumped into one is basically
Oh, except that rpmugGname() and rpmugUname() additionally rely on central
storage of the returned values so simple mutex locking doesn't work for those.
So those would have to be changed to return malloced data for a simple fix (it
wont break any users because there aren't any, at the moment).
@pmatilai pushed 3 commits.
4945ad298058fd3c325c6b803ef6c3e2cb2d97aa Remember to free user/group cache on
librpm shutdown (again)
8656e2e0327a9af38ef180eb014bac9271b3f8df Centralize user/group lookup caching
into a single data structure
d23eb53abdde25ad2c45d20c32dde255fb36384d Make user/group
Split to refactor + thread-safe fix, and restored that rpmugFree() on librpm
exit that had gone missing at some point.
--
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/2843#issuecomment-1893715068
You are receiving this because you are sub
Looks good to me.
--
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/2843#issuecomment-1896120720
You are receiving this because you are subscribed to this thread.
Message ID: ___
Rpm-maint mailing
@pmatilai pushed 1 commit.
58b13066e4b540d6440c41becaf3690663cd46d2 Simplify the cache lookup logic, no
functional changes
--
View it on GitHub:
https://github.com/rpm-software-management/rpm/pull/2843/files/d23eb53abdde25ad2c45d20c32dde255fb36384d..58b13066e4b540d6440c41becaf3690663cd46d2
You
@dmnks commented on this pull request.
>
return 0;
}
const char * rpmugUname(uid_t uid)
{
-static uid_t lastUid = (uid_t) -1;
-static char * lastUname = NULL;
-
-if (uid == (uid_t) -1) {
- lastUid = (uid_t) -1;
I wonder if this line shouldn't be kept? `rpmugFree()`
It seems like the commit hash mentioned in the commit message isn't correct
(it's the "Bump CI" commit which doesn't seem to have anything to do with this).
--
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/2843#issuecomment-1898909235
You
@dmnks commented on this pull request.
>
return 0;
}
const char * rpmugUname(uid_t uid)
{
-static uid_t lastUid = (uid_t) -1;
-static char * lastUname = NULL;
-
-if (uid == (uid_t) -1) {
- lastUid = (uid_t) -1;
Ah, this is not needed now that we have a `struct`, of
@dmnks approved this pull request.
Other than the commit note above, the patches look good to me.
--
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/2843#pullrequestreview-1830246698
You are receiving this because you are subscribed to thi
@pmatilai commented on this pull request.
>
return 0;
}
const char * rpmugUname(uid_t uid)
{
-static uid_t lastUid = (uid_t) -1;
-static char * lastUname = NULL;
-
-if (uid == (uid_t) -1) {
- lastUid = (uid_t) -1;
Yup, and as an added bonus it now never contains an
@Conan-Kudo approved this pull request.
Modulo note as @dmnks stated, it looks good to me.
--
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/2843#pullrequestreview-1833116074
You are receiving this because you are subscribed to this threa
> It seems like the commit hash mentioned in the commit message isn't correct
> (it's the "Bump CI" commit which doesn't seem to have anything to do with
> this).
It's actually explained in the message:
> rpmugUname() and rpmugGname() are have no users in the current codebase,
> so this was de
Merged #2843 into master.
--
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/2843#event-11555214102
You are receiving this because you are subscribed to this thread.
Message ID:
___
Rpm-maint mail
21 matches
Mail list logo
