On 5/17/19 10:52 AM, Florian Weimer wrote:
* Carlos O'Donell:
> # XXX: Ideally ld.so.conf should have the timestamp of the spec file, but there
> # doesn't seem to be any macro to give us that. So we do the next best thing,
> -# which is to at least keep the timestamp consistent. The choice of using
> -# glibc_post_upgrade.c is arbitrary.
> +# which is to at least keep the timestamp consistent. The choice of using
> +# %{SOURCE0} is arbitrary.
> touch -r %{SOURCE0} %{glibc_sysroot}/etc/ld.so.conf
> touch -r sunrpc/etc.rpc %{glibc_sysroot}/etc/rpc
This should use SOURCE0 in the comment, to avoid a lint warning due to a
macro in a comment.
Fixed.
> # All of the bin and certain sbin files go into the common
package except
> -# glibc_post_upgrade.* and iconvconfig which need to go in glibc. Likewise
> -# nscd is excluded because it goes in nscd.
> +# iconvconfig which needs to go in glibc. Likewise nscd is excluded because
> +# it goes in nscd.
> grep '%{_prefix}/bin' master.filelist >> common.filelist
> -grep '%{_prefix}/sbin/[^gi]' master.filelist \
> +grep '%{_prefix}/sbin/iconvconfig' master.filelist \
> | grep -v 'nscd' >> common.filelist
Shouldn't this be echo instead of grep
Worse, this is a bug, it moves only iconvconfig into common.fileslist, and I
missed this because I didn't do a file-by-file-by-rpm comparison.
The previous grep is '^gi' which is to say "Anything that doesn't start
with g
or i" so the we do want grep, but we want:
I've fixed this now.
I did a file-by-file-by-rpm comparison and it was clean (should do this every
time I touch these list edit steps).
> +%post -p <lua>
> +-- (1) Remove multilib libraries from previous installs.
> +-- In order to support in-place upgrades, we must immediately remove
> +-- obsolete platform directories after installing a new glibc
> +-- version. RPM only deletes files removed by updates near the end
> +-- of the transaction. If we did not remove the obsolete platform
> +-- directories here, they would be preferred by the dynamic linker
> +-- during the execution of subsequent RPM scriptlets, likely
> +-- resulting in process startup failures. */
> +
> +-- We are going to remove these libraries. Generally speaking we remove
> +-- all core libraries in the multilib directory.
> +local remove_prefixes = { "libc-",
> + "libm-",
> + "librt-",
> + "libpthread-",
> + "librtkaio-",
> + "libthread_db-" }
Should this be more restrictive, eventually generating a match pattern
like "libc%-[1-9][0-9]*%.[0-9]+%.so"?
The original code was more relaxed than your regexp, since it allowed
[.0-9] in any order at all in the characters between the prefix and the
ending '.so' suffix.
My new code is even more relaxed, allowing prefix.*\.so.
I will adjust it to use a fixed regexp to minimize problems with potential
future multilibs from other projects.
Fixed.
Or should we reserve this for a future change? It would allow us to
fix
downgrades if we nuke additional library copies that have names that do
not match the current library.
I will restrict the match as you wrote above, but no further for now.
I think we can revisit non-multilib downgrades later.
> +-- (2) Update /etc/ld.so.conf
> +-- Next we update /etc/ld.so.conf to ensure that it starts with
> +-- a literal "include ld.so.conf.d/*.conf".
> +
> +local ldsoconf = "/etc/ld.so.conf"
> +local ldsoconf_tmp = "/etc/glibc_post_upgrade.ld.so.conf"
> +
> +local conf_fd = io.open (ldsoconf, "r")
> +
> +if conf_fd ~= nil then
> +
> + -- We must have a "include ld.so.conf.d/*.conf" line.
> + local have_include = false
> + for line in conf_fd:lines () do
> + -- This must match, and we don't ignore whitespace.
> + if string.match (line, "^include ld.so.conf.d/%*%.conf$") ~= nil then
> + have_include = true
> + end
> + end
> + conf_fd:close ()
I think you could io.lines here as well (because you do not use break).
Simplified then to use posix.access and io.lines.
Fixed.
> +-- (3) Rebuild ld.so.cache early.
> +-- If the format of the cache changes then we need to rebuild
> +-- the cache early to avoid any problems running binaries with
> +-- the new glibc.
> +
> +if posix.access ("%{_prefix}/sbin/ldconfig", "x") then
Not sure why the access check is needed here?
The original C code says this:
-+ /* If installing bi-arch glibc, rpm sometimes doesn't unpack all files
-+ before running one of the lib's %post scriptlet. /sbin/ldconfig will
-+ then be run by the other arch's %post. */
This seems like gibberish. RPM always unpacks all the files. It has to.
I'm removing it, we don't do any similar check for iconvconfig, we
expect it's installed and executable or we fail.
Fixed.
> + local pid = posix.fork ()
> + if pid == 0 then
> + posix.exec ("%{_prefix}/sbin/ldconfig")
Suggest: assert(posix.exec ("%{_prefix}/sbin/ldconfig")), to terminate
execution in the subprocess on error. But this a bit tricky.
Fixed. I'm using assert for ldconfig and iconvconfig.
> + elseif pid > 0 then
> + posix.wait (pid)
> + end
> +end
Maybe add a comment that you use posix.exec to avoid the use of the
shell, which may not be installed yet?
Fixed.
You should put this into a function, to reuse it below.
Fixed.
v2
- Fixed SOURCE0 reference in comment.
- Fixed iconvconfig inclusion in glibc.filelist.
- Fixed iconvconfig removal from common.filelist.
- Created post_exec function to run execs with asserts to check for errors.
- Switched step 1 to use tighter regexps.
- Switched step 1 to use %{_libdir} where possible.
- Fixed step 2 to use io.lines and simplify.
- Always run ldconfig in step 3 using post_exec
- Step 4 uses post_exec also.
That resolves all the immediate issues you and DJ raised.
Comments appreciated on v2.
--
Cheers,
Carlos.