On Sun, Aug 29, 2010 at 12:46:52PM +0100, Roger Leigh wrote:
> On Sun, Aug 29, 2010 at 12:07:22PM +0200, Marc 'HE' Brockschmidt wrote:
> > Roger Leigh <rleigh@codelibre.net> writes:
> > > On Sat, Aug 28, 2010 at 01:33:10PM +0200, Marc 'HE' Brockschmidt wrote:
> > >> Roger Leigh <rleigh@codelibre.net> writes:
> > >>> Please could you unblock schroot 1.4.10-2?
> > >> 127 files changed, 6132 insertions(+), 6896 deletions(-)
> > >>
> > >> *cough* This is a bit much and far away from properly reviewable - even
> > >> if we filter out all autoconf crop and documentation updates. Are all of
> > >> these changes really needed?
> > > Yes. If you look at the diffstat (below),
> >
> > We always do that when reviewing changes. In this case, this ends up with
> > he@franck:~$ filterdiff -i *.cc -i *.h < schroot-diff | diffstat
> > [...]
> > 79 files changed, 1255 insertions(+), 726 deletions(-)
> >
> > As schroot is an important part of the Debian infrastructure, I'm even
> > more reluctant to just accept these changes into stable. How have these
> > changes been tested?
>
> I've done hand testing of all changes myself in addition to testing
> with the included testsuite. […]
> I've also tested it with sbuild, and all the releases are all
> known to function correctly when run with sbuild.
>
> Releases later than 1.4.7 also deprecate features for removal in
> squeeze+1. If we don't have an update in squeeze, I'll need to
> postpone some changes until squeeze+2 which I would really prefer
> not to do.
I'm afraid I've been ill for the last few days, but what I have done
is to separate the big diff into separate sets of changes, which
means you can look at things with most of the "noise" taken out.
The patches are available from
http://people.debian.org/~rleigh/schroot-1.4.11-patches/
Summary of each patch:
race-fix.diff
Work around a kernel bug
Fixes bugs: #593516
Changes
• Use an flock file lock when reading /proc/mounts to reduce the
change of racing reading /proc/mounts, which could potentially
lead to catastrophic dataloss if we rm -rf a mounted filesystem
the kernel skipped when reading /proc/mounts. This isn't a true
fix, since programs mounting/unmounting without taking the same
lock will still trigger this, but the chances are vastly reduced.
Required: Yes. This is a very serious bug, and needs fixing for
squeeze. Chance of triggering is low, but if running many parallel
schroot instances, the chance of beginning/ending a session at the
same time is greatly increased. Independently tested, and tested by
me.
session-name-check.diff
Add strict checking for chroot names
Fixes bugs: #589889
Changes
• Add is_valid_sessionname function; based on is_valid_filename
Required: Yes. Closes potential security hole by preventing session
names from containing relative pathnames which could be abused to
overwrite files with root privileges. 1.4.7 is not vulnerable, but
uses a rather stricter check which means many valid session names
are not permitted. Tested.
environment.diff
Correctly set up user environment in chroot
Fixes bugs: #589830, #589917
Changes:
• sbuild::auth::get_minimal_environment unconditionally preserves
TERM and SHELL in the environment. LOGNAME and USER are also
unconditionally preserved.
Severity: very important (bad environment under some circumstances,
fixes a regression introduced in 1.4.7)
Required for squeeze: Yes (1.4.7 is buggy)
chroot-namespace.diff
Fixes bugs: #512131
Addition of support for chroot namespaces.
Changes
• sbuild::chroot_config methods now include a "namespace" argument
• Validate session names (for security; see above)
• New functions to search namespaces (find*/lookup*)
• Chroot listing/info functions also print namespaces
• Add compatibility aliases to preserve full backward compatibility
with earlier releases
Required: No. However, this feature has been long requested by
several buildd admins (to allow all source chroots to be updated
using the accompanying --all-source-chroots option (below) which
this enables). If it's not in squeeze, we won't be able to update
dependent tools and deprecate the compatibility aliases until
squeeze+1, which is IMO too long for a relatively simple change.
Most changes here are simply adding a chroot_namespace parameter
which touches most functions, and adjusting existing code to use the
new find_*/lookup_* functions. This has been extensively tested.
namespace-session.diff
Session support for chroot namespaces
Fixes bugs: N/A
Changes
• Don't use a copy of the chroot_config; pass in chroot objects
directly. This means a number of failure modes are no longer
possible, increasing robustness and security. Replace string_list
with chroot_list
• Remove a number of static assertions and checks which are no
longer required.
• Changes to dchroot session classes; update to use updated
interfaces
Required: It's a prerequisite for namespace support. Tested.
schroot-options.diff
Addition of --all-source-chroots option for schroot
Fixes bugs: N/A
Changes
• Add all_source_chroots to schroot options and --all-source-chroots
command-line option to set it; update option validation to work
with the new option
• When loading schroot.conf, use namespaces.
• Use new chroot config search methods when setting up chroots for
use
• Use new chroot config print methods when displaying chroot info
• Use new schroot chroot list interface
Required: It's a prerequisite for namespace support. Tested.
test.diff
Testsuite updates
Fixes bugs: N/A
Changes
• Updates to tests for chroot namespaces
Required: It's a prerequisite for namespace support
dchroot-deprecated.diff
Don't use deprecated keyfile names.
Fixes bugs: N/A
Changes
• Use "directory" rather than "location" when creating schroot
configuration from dchroot.conf
Required: No. However, this removes lots of annoying deprecation
warnings which the user can do nothing to correct. Trivial one-
liner change which is demonstrably correct (and has been tested
to be so).
device-type-refactor.diff
Move virtual method into a different file
Fixes bugs: N/A
Changes
• Move get_chroot-type method from sbuild::chroot_block_device_base
to sbuild::chroot_block_device, which is where it belongs since
sbuild::chroot_block_device_base is not an instatiatable type.
Required: No. Not strictly a bug, but was not good practice.
Tested.
namespace-remove.diff
Code cleanup
Fixes bugs: N/A
Changes
• Removal of unnecessary use of sbuild:: namespace
Required: No. Changes are simple search-and-replace which gives
a big diff but isn't actually changing anything. Tested.
debian.diff
Updates to Debian packaging
Fixes bugs: N/A
Changes
• Separate doxygen documentation building so that it's not build
redundantly for all arches by only building in a build-indep rule.
Required for squeeze: Possibly not, but schroot 1.4.7 will not
autobuild currently due to doxygen bugs this works around. Tested
successfully from 1.4.10-1 to 1.4.11.
session-name-unify.diff
Unify chroot/session/keyfile names
Fixes bugs: #593256
Changes
• Unify chroot get_name/get_session_name/get_keyfile_name methods
with single get_name method.
• All functions using get_session_name/get_keyfile_name updated to
use get_name
• Remove get_active and use get_facet<chroot_facet_session> to
check for presence of session facet [to check if chroot is session
chroot]
Required: No. This is straightforward search-and-replace code
refactoring, so should be a safe change. Makes the code far more
clean and robust. Tested.
po.diff
Translation updates
Fixes bugs: #594024 #594239 #593622 #588734 #588963 #589079
Changes
• Updates for various translations
• Removal of file/line numbers to make translation updates more
diff-friendly in the future.
Required for squeeze: Yes.
build.diff
Changes to build intrastructure
Fixes bugs: #589658
Changes
• Addition to doc rule to top-level Makefile for building doxygen
docs and updates to improve documentation building
• Bump of version number
• Autodetection of doxygen
• Correct autodetection of LVM, Btrfs and losetup
• Correct Boost program_options autodetection
Required for squeeze: Yes. The configure changes are required for
correct feature autodetection, which can otherwise fail and
result in a misbuild. Tested.
doc.diff
Changes to documentation (no code changes)
Fixes bugs: N/A
Changes
• Update NEWS (chroot namespaces and feature deprecations)
• Update README (document configure options)
• Doxygen template update
• ChangeLog update
• schrootl.conf template update
Required for squeeze: N/A. This is just documentation accompanying
the rest of the changes.
doxygen.diff
Updates to API reference (no code changes)
Fixes bugs: N/A
Changes
• Updates to inline doxygen code comments
Required for squeeze: N/A. This is just documentation accompanying
the rest of the changes. This updates the API reference to give
the documentation 100% code coverage.
#595647 is the only outstanding issue (and it's not a regression,
just behaviour not matching documentation), and is just a two-liner
to fix.
Regards,
Roger
--
.''`. Roger Leigh
: :' : Debian GNU/Linux http://people.debian.org/~rleigh/
`. `' Printing on GNU/Linux? http://gutenprint.sourceforge.net/
`- GPG Public Key: 0x25BFB848 Please GPG sign your mail.
Attachment:
signature.asc
Description: Digital signature