Page MenuHomeFreeBSD
Differential D58165

pkg: Add -j and -r options
AcceptedPublic
Actions

Authored by des on Fri, Jul 10, 7:54 PM.
Tags
None
Referenced Files
F164011527: D58165.id182593.diff
Mon, Jul 27, 9:37 PM
F163993128: D58165.id182297.diff
Mon, Jul 27, 5:14 PM
F163969401: D58165.diff
Mon, Jul 27, 1:54 PM
F163942567: D58165.id182392.diff
Mon, Jul 27, 8:47 AM
F163941052: D58165.id182391.diff
Mon, Jul 27, 8:30 AM
F163939494: D58165.id181914.diff
Mon, Jul 27, 8:13 AM
F163938186: D58165.id182297.diff
Mon, Jul 27, 7:58 AM
Unknown Object (File)
Sun, Jul 26, 3:18 AM
View All 46 Files
Subscribers
imp
markj
ziaee

Details

Reviewers
bapt
bdrewery
imp
markj
Group Reviewers
srcmgr
Commits
rGd94e034d5046: pkg: Add -j and -r options
Summary

This allows pkg(7) to be used to bootstrap a chroot or jail, and to
recognize the -j and -r options and pass them through to pkg(8) if
already bootstrapped.

The -j option greatly complicates the option-parsing loop because we
can no longer pass our original argv unmodified to pkg(8); specifcially,
we must strip -j and its argument, which in practice means constructing
a whole new argv with the options we wish to pass on.

Note that this does not address the issue of repository keys. If using
a signed package repository, you will still need to copy /usr/share/keys
into the target environment before or after bootstrapping, or pkg will
be unable to verify package signatures.

MFC after: 1 week

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 74954
Build 71837: arc lint + arc unit

Event Timeline

des created this revision.Fri, Jul 10, 7:54 PM
Herald added a reviewer: bdrewery. · View Herald TranscriptFri, Jul 10, 7:54 PM
Herald added subscribers: ziaee, imp. · View Herald Transcript
des requested review of this revision.Fri, Jul 10, 7:54 PM
Harbormaster completed remote builds in B74712: Diff 181752.Fri, Jul 10, 7:54 PM
des added a child revision: D57956: pkgbasify: Add new utility.Fri, Jul 10, 7:55 PM
imp accepted this revision.Sun, Jul 12, 5:16 PM

I like it.

This revision is now accepted and ready to land.Sun, Jul 12, 5:16 PM
des retitled this revision from pkg: Add support for -r option to pkg: Add support for -r and -j.Mon, Jul 13, 12:57 PM
des edited the summary of this revision. (Show Details)
des updated this revision to Diff 181902.Mon, Jul 13, 12:57 PM

and -j

This revision now requires review to proceed.Mon, Jul 13, 12:57 PM
Harbormaster completed remote builds in B74768: Diff 181902.Mon, Jul 13, 12:58 PM
bapt accepted this revision.Mon, Jul 13, 2:59 PM
This revision is now accepted and ready to land.Mon, Jul 13, 2:59 PM
Closed by commit rGd94e034d5046: pkg: Add -j and -r options (authored by des). · Explain WhyMon, Jul 13, 3:34 PM
This revision was automatically updated to reflect the committed changes.
des added a commit: rGd94e034d5046: pkg: Add -j and -r options.
markj added a subscriber: markj.Mon, Jul 13, 3:36 PM
markj added inline comments.
usr.sbin/pkg/pkg.c
96

Why is rootd a global variable? And shouldn't it be initialized to AT_FDCWD?

kevans added a reverting change: rG9dfaf1cb37f8: Revert "pkg: Add -j and -r options".Mon, Jul 13, 10:09 PM
des reopened this revision.Sun, Jul 19, 7:06 AM
des retitled this revision from pkg: Add support for -r and -j to pkg: Add -j and -r options.
des edited the summary of this revision. (Show Details)
des updated this revision to Diff 182202.Sun, Jul 19, 7:06 AM

we always need rootd

Harbormaster completed remote builds in B74926: Diff 182202.Sun, Jul 19, 7:06 AM
des marked an inline comment as done.Sun, Jul 19, 7:07 AM
markj added inline comments.Sun, Jul 19, 4:52 PM
usr.sbin/pkg/pkg.c
96

It still doesn't need to be global AFAICS.

264

If the assertion is false, then the program has already triggered some memory corruption and the assertion might not catch it.

287
290

This will always be zero.

1149

What if localbase is just "/"? I think this construction doesn't quite work in that case, pkgpatch will contain "//sbin/pkg" and the +1 in the faccessat() call below won't behave as you want.

If localbase is the empty string, this also doesn't work, but I think that probably shouldn't happen.

des updated this revision to Diff 182252.Sun, Jul 19, 7:34 PM

Turns out this is a lot more complicated than I thought

Harbormaster completed remote builds in B74937: Diff 182252.Sun, Jul 19, 7:34 PM
des added a child revision: D58349: pkg: Add tests.Sun, Jul 19, 7:36 PM

See also D58348 and D58349

des updated this revision to Diff 182291.Mon, Jul 20, 3:13 PM

rf

Harbormaster completed remote builds in B74954: Diff 182291.Mon, Jul 20, 3:13 PM
des marked 2 inline comments as done.Mon, Jul 20, 5:19 PM
des updated this revision to Diff 182297.Mon, Jul 20, 5:25 PM

remove unnecessary exit status checks

Harbormaster completed remote builds in B74960: Diff 182297.Mon, Jul 20, 5:25 PM
des marked 2 inline comments as done.Mon, Jul 20, 5:28 PM
des added inline comments.Tue, Jul 21, 4:11 AM
usr.sbin/pkg/pkg.c
1149

If localbase is an empty string then pkgpath will be /sbin/pkg which is fine, I guess. See also D58362 which adds basic normalization to getlocalbase().

markj accepted this revision as: markj.Tue, Jul 21, 2:05 PM
markj added inline comments.
usr.sbin/pkg/pkg.c
1143
1347

In pkg(8), -d isn't a boolean flag, repeating it increments DEBUG_LEVEL.

This revision is now accepted and ready to land.Tue, Jul 21, 2:05 PM
des updated this revision to Diff 182391.Tue, Jul 21, 4:38 PM

rf

This revision now requires review to proceed.Tue, Jul 21, 4:38 PM
Harbormaster completed remote builds in B74984: Diff 182391.Tue, Jul 21, 4:38 PM
des updated this revision to Diff 182392.Tue, Jul 21, 4:39 PM

rf

Harbormaster completed remote builds in B74985: Diff 182392.Tue, Jul 21, 4:39 PM
markj accepted this revision as: markj.Tue, Jul 21, 4:40 PM
This revision is now accepted and ready to land.Tue, Jul 21, 4:40 PM
des marked 3 inline comments as done.Tue, Jul 21, 4:47 PM
des updated this revision to Diff 182393.Tue, Jul 21, 4:49 PM

getlocalbase() normalizes now

This revision now requires review to proceed.Tue, Jul 21, 4:49 PM
Harbormaster completed remote builds in B74986: Diff 182393.Tue, Jul 21, 4:49 PM
des updated this revision to Diff 182593.Fri, Jul 24, 4:55 AM

copy argv while parsing it instead of trying to reconstruct it later

Harbormaster completed remote builds in B75078: Diff 182593.Fri, Jul 24, 4:55 AM
des edited the summary of this revision. (Show Details)Sat, Jul 25, 5:47 PM
des updated this revision to Diff 182698.Sat, Jul 25, 6:06 PM

refine

Harbormaster completed remote builds in B75126: Diff 182698.Sat, Jul 25, 6:06 PM
markj accepted this revision as: markj.Mon, Jul 27, 2:00 PM
markj added inline comments.
usr.sbin/pkg/pkg.c
1157

A general comment: I think the proliferation of -j <jail to attach to> flags is bad. Attaching to a jail isn't trivial, you should at least make sure that all your (maybe inherited) directory descriptors are closed first. Otherwise someone can ptrace your process after it attaches and access directories outside the chroot. I understand that pkg(8) already sports this flag, I just wonder why jexec(8) isn't good enough.

A side note: jail_attach() changes the process cwd. So, if someone specifies -r and -j, the rootdir might be interpreted relative to the jail root or not, depending on the order they are processed. I'm not sure how pkg(8) handles this, but we should aim to be compatible.

This revision is now accepted and ready to land.Mon, Jul 27, 2:00 PM

Revision Contents
Changeset List

PathSize
usr.sbin/
pkg/
2 lines
28 lines
161 lines

Diff 182291

View Options

usr.sbin/pkg/Makefile

Loading...
View Options

usr.sbin/pkg/pkg.7

Loading...
View Options

usr.sbin/pkg/pkg.c

Loading...