Page MenuHomeFreeBSD

Installer Enhancement -- dpv
ClosedPublic

Authored by dteske on Sep 2 2014, 11:07 PM.

Details

Summary

In bsdinstall's distextract, replace mixed_gauge() of dialog(3) with
new dpv(3) wrapper to dialog(3) dialog_gauge(). The dpv(3) library provides
a more flexible and refined interface similar to dialog_mixedgauge() however
is implemented atop the more generalized dialog_gauge() for portability.
Noticeable improvements in bsdinstall's distextract will be a status line
showing data rate information (with support for localeconv(3) to format
numbers according to $LANG or $LC_ALL conversion information), i18n support,
improved auto-sizing of gauge widget, a ``wheel barrow'' to keep the user
informed that things are moving (even if status/progress has not changed),
improved color support (mini-progress bars use the same color, if enabled,
as the main gauge bar), and several other improvements (some not visible).
dpv stands for "dialog progress view" (dpv was introduced in SVN r274116).

Discussed on: -current

Test Plan

Ensure that distribution unpack works in combinations of:
+ Installing via serial connection
+ Installing minimal distribution set (fewest dists)
+ Installing maximal distribution set (most dists)
+ Installing with alternate keymap and/or language region(s)

Diff Detail

Repository
rS FreeBSD src repository
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

dteske updated this revision to Diff 1337.Sep 2 2014, 11:07 PM
dteske retitled this revision from to Installer Enhancement -- dpv.
dteske updated this object.
dteske edited the test plan for this revision. (Show Details)
dteske added reviewers: julian, adrian, emaste.
ngie added a subscriber: ngie.Sep 2 2014, 11:39 PM

I got about halfway through the review. I'll take a look at the second half later. I really like the documentation -- thanks :)!!!

lib/libdpv/dpv.c
30–42 ↗(On Diff #1337)

style(9): the headers need to be sorted

61–62 ↗(On Diff #1337)

If FALSE is 0, then this will be taken care of when the program is loaded as all BSS values are initialized to 0 (which includes NULL).

91–112 ↗(On Diff #1337)

style(9): these variables should be stack-aligned, pointers first, scalars last, largest to smallest.

139–140 ↗(On Diff #1337)

The braces for multi-line conditional and loop blocks should be done like:

if (config != NULL) {
153 ↗(On Diff #1337)

style(9):

  1. single-line statements need to be on separate lines.
  2. Return values need to be wrapped in ellipses.

i.e.

if (pprompt == NULL)
    return (-1);
159–162 ↗(On Diff #1337)

Spurious whitespace between the "=".

209–213 ↗(On Diff #1337)

style(9): please indent the code one more block here

328 ↗(On Diff #1337)

Not sure if this needs space or "(" / ")" around the assignment ternary per style(9)...

585 ↗(On Diff #1337)

Why 512?

598 ↗(On Diff #1337)

Should the return code be checked?

lib/libdpv/dpv_private.h
61 ↗(On Diff #1337)

This should be implied via ANSI-C.

lib/libdpv/util.c
41 ↗(On Diff #1337)

This should be implied with BSS initialization.

51–52 ↗(On Diff #1337)

Please have jilles@/someone from security@ review this really quickly.

I don't remember whether there's an alternative to doing this or not in libc, but this kind of concerns me because there are string quoting issues at least with cmd.

109 ↗(On Diff #1337)

Memory leaked intentionally?

julian edited edge metadata.Sep 3 2014, 9:28 AM

seems to act as advertised. maybe it will give a common feel to a bunch of tools.. I think you said ti would work with Xdialog too? or was that my imagination?

lib/libdpv/dpv.c
159–162 ↗(On Diff #1337)

I'll just reply to a single style comment to say that really this is close, but that yes, I noticed the things being pointed out.. I assume the indents are tabs and that phabricator is just showing them as about 2 spaces.

usr.bin/dpv/dpv.1
310 ↗(On Diff #1337)

nice examples.. I had a play with them on the machine I set up using the demo installer.

julian accepted this revision.EditedSep 3 2014, 9:30 AM
julian edited edge metadata.

oh yeah.. I have no real problems with it. only the style nits pointed out elsewhere.

ok by me when you get OKs from people more into the code in question

This revision is now accepted and ready to land.Sep 3 2014, 9:30 AM
dteske updated this revision to Diff 1397.Sep 8 2014, 3:49 AM
dteske edited edge metadata.
  1. Updating D714: Installer Enhancement -- dpv #
  2. Enter a brief description of the changes included in this update.
  3. The first line is used as subject, next lines as comment. #
  4. If you intended to create a new revision, use:
  5. $ arc diff --create

style(9) and memory mgmt updates/changes:
Change: Add dialog_maxsize_free() and hook it into dpv_free(3)
Change: Add dialogrc_free() and hook it into dpv_free(3)
Change: Add dprompt_free() and hook it into dpv_free(3)
Change: Add some comments
Change: Add status_free() and hook it into dpv_free(3)
Change: Assign NULL back to free'd pointers
Change: Call dpv_free(3) before returning from main() in dpv(1)
Change: Change a statically-sized array into such instead of using malloc
Change: Change msg_ into mesg_ to align with {done,fail,pend}_
Change: Combine separate if-statements where possible
Change: Document an interesting bug in dpv(1) manual
Change: Document an interesting bug in dpv(3) manual
Change: Don't reinvent {BEGIN,END}_DECLS from <sys/cdefs.h>
Change: Fix a valid warning observed under ``clang -Weverything''
NB: Added explicit typecast from off_t to uint32_t
NB: Where off_t is communicated by clang as `long long'
Change: Fix typo(s) in usage statement
Change: Remove
cplusplus in header lacking function declrations
Change: Typecast to void when tossing non-void function results
Change: Use TRUE/FALSE not 0/1 for boolean ``nls'' (newline-state var)
Change: Whitespace
Recommended: Don't declare variables locally in sub-blocks
NB: Declare them atop the function unless the function is really complex
NB: To prevent masking of prior declarations
Recommended: break variable declarations into single-lines
style(9): Don't include <sys/cdefs.h> when including <sys/types.h>
style(9): Don't treat bitwise operand results as boolean in if-stmts
style(9): Don't treat numbers as boolean in if-statements
style(9): Functions declaring no variables, blank line after open curly
style(9): Kernel include files (i.e. sys/*.h) come first
style(9): Make functions private to one source file static
style(9): Move ``lonely curlies'' to the EOL of previous line
NB: For while-loops, if-statements, for-loops, etc.
style(9): Re-align function declarations in header file(s)
style(9): Remove unnecessary parentheses around ternary operator
NB: When used as an rvalue assignment without math and/or as a
function argument -- similarly if without math performed on result
style(9): Separate kernel includes (sys/*.h) from other /usr/include files
style(9): Separate multi-statement lines
style(9): Separate single-action if-line and action-line
style(9): Sort function declarations by return size first then alpha
style(9): Sort function prototypes by return type size first then alpha
style(9): Sort include headers
style(9): Sort variable declarations by size then alphabetically
style(9): Surround return statement with parentheses
style(9): Use /* FALLTHROUGH */ and /* NOTREACHED */ where applicable
style(9): Use a single tab to align values
style(9): Usually sys/types.h or sys/param.h first (never both)

dteske added a comment.Sep 8 2014, 4:08 AM

Addressed many issues in Diff #1397 -- mostly style(9) and memory management related.

lib/libdpv/dpv.c
30–42 ↗(On Diff #1337)

Addressed in Diff #1397

61–62 ↗(On Diff #1337)

Discussed with julian@ and he recommended keeping the explicit initial assignments.

91–112 ↗(On Diff #1337)

Addressed in Diff #1397

139–140 ↗(On Diff #1337)

Addressed in Diff #1397

153 ↗(On Diff #1337)

Addressed in Diff #1397

159–162 ↗(On Diff #1337)

Not spurious whitespace; aligning rvalues.

159–162 ↗(On Diff #1337)

Addressed in Diff #1397

209–213 ↗(On Diff #1337)

You cite style(9) here, but style(9) actually says:

switch (ch) { /* Indent the switch. */
case 'a': /* Don't indent the case. */

aflag = 1;      /* Indent case body one tab. */

I observe proper switch indentation according to style(9).
NB: style(9) r217087

328 ↗(On Diff #1337)

Addressed in Diff #1397

585 ↗(On Diff #1337)

To keep the randomized fake readout in the megabytes range. Comment added in Diff #1397

598 ↗(On Diff #1337)

Looked at the man-page. Do you recommend an err(3) call if it returns bad value? or what?

lib/libdpv/dpv_private.h
61 ↗(On Diff #1337)

It may be implied, but it's very VERY important that it be explicitly stated so that you _know_ that performing a memset to zero the config struct will cause certain behavior (that by looking at the header, you should be able to immediately tell what the behavior will be for a NULL-ified config struct -- stating the first element is zero helps visually make that connection for me).

lib/libdpv/util.c
41 ↗(On Diff #1337)

Discussed with julian@ and we both think it helps to keep the initializer.

51–52 ↗(On Diff #1337)

Sure, no harm in having him take a second look.

109 ↗(On Diff #1337)

Yes, haven't found a way to prevent the segmentation fault that arises from attempting to free the positional argument array (even if waiting until after the spawned child process has closed). Minimal impact, but for good measure, I addressed in Diff #1397 additional memory management issues.

What else is this library for? I don't necessarily have any objections to the patch per se, but it's a huge amount of extra code to this otherwise very simple libarchive frontend to add an extract speed status bar, which for extraction (and opposed to downloading files) seems like something of pretty marginal utility.

ngie added a comment.Sep 11 2014, 10:13 PM

I agree with nathanw@ about the complexity. It's best to avoid feature creep in the installer; otherwise bsdinstall will turn into sysinstall.

Please see if you can add kientzle@ to review the libarchive code (he's not a Phabric user, so you'll need to prod him about creating an account).

It seems like distextract could be replaced with a much simpler shell script; if so, would it render the dpv work unnecessary or could it instead use the dpv program [directly/indirectly] to better abstract out the MVC nature of the code you're trying to add?

[this applies to the new code as well as the existing code]
Did you find this thread that kientzle@ responded to: https://groups.google.com/forum/#!topic/libarchive-discuss/0MWyuZafnpM ? It

I didn't do a shell script in the beginning because it was hard to feed status information from tar into dialog in a sane way. Since the alternative 160 lines of C didn't require gymnastics, that seemed like the safer and more maintainable approach. distfetch is in C for the same reason. Any approaches that are even shorter/more robust would be good alternatives to the existing code.

Nathan, I do understand your thoughts about whether adding a library just for the installer and configuration tool is worth it, but then I found on trying it out that I think the responsiveness and improved feedback made the install just that bit nicer. The code is there, and its written, and it has a maintainer.
I know that dialog is buggy in many cases and having a pre-canned way to use it would have saved me much pain several years back when I needed it, so I sort of hope that having this library available to work with dialog might actually eventually simplify the base application a bit . The hidden parts of this are of course that when the library is available, it works with xdialog as well and gives a unified look and feel to both environments.

The whole point of the dpv program is to provide a flexible and generally useful tool to provide feedback for un-named applications.. sort of "the unix way". You pipe it up with other things to get a status bar. it was I believe written largely with a commercial application installation in mind, but we get the use of it for free.
To some extent it's a chicken and egg situation.. until such a library is in place nothing will use it and until there are several users of such a library, you don't have a reason to put such a library in place. Devin has 3 places he's involved with that he wants to use this in.. installer, configurator, and in the way packages are handled under frontends. but until he has it somewhere he can't.

chicken <--> egg

Let me give some personal examples.. before I could put netgraph in the system including libnetgraph and the ngctl program, there were complaints of "what, a whole new kernel directory, and new library and two new programs? just for a packet munger?". Now there are over 50 netgraph node types and it's used in all sorts of places. The pushback was quite vocal.. (particularly from Scandinavia :-)
pretty much the same for "why are you smashing up the network stack and putting all those extra commands in ipfw, route, netstat etc.? ". Now "use separate fibs" or "use a vnet jail" are common answers to the more curly networking questions that come up..
Ditto for devfs. (even tough it was eventually rewritten on the experience of the one I originally wrote), "what an entire new filesystem to handle devices?"... and on each one I had to seriously consider whether the additional stuff I wanted to add was "bloat" or "added functionality".
In this case since we already have a version of the dialog code in the tree just for the install, etc. I think dropping the dpv program (and thus it's library as well) along side it is not a great problem. I'm not sure if I'd hide the lib code inside the context of the installer and move it out when other users materialise, or put it out in the plain firing line in its own directory. I suspect that personally I'd do the first, but still make sure that it was easily understood how to access it from a 2nd application. later if there are more users it could be moved out. Either would be fine for me.

Julian

Julian, I agree. That's why I first asked what else this was for. If it's a general purpose tool that adds really useful features and this is the first use, that's absolutely fine. It would be nice to work with upstream to make this a part of libdialog itself, though, rather than wrapping it.

What in general I don't want to happen is that the installer becomes progressively more and more complex with little shared code with the rest of the system. It lives in a really awkward place in the software stack where it absolutely must be bug-free and simultaneously is almost never tested since it's a program that people use once. Introducing crippling regressions here is thus really easy; the only way around that is to be sure that as much code as possible is shared with tools that people do use every day. There's not that much regression risk here, since this is just tar with progress bars and is hard to get wrong in subtle ways, but that's anyway why I'm concerned to see multi-thousand line diffs to a 150-line program.

One note on the code itself. I don't know if this is possible, but it might also be nice to simplify the API a little: interacting with libdpv rather than libdialog nearly doubles the total code length of distextract in this patch.

dteske added a comment.EditedSep 12 2014, 7:56 PM
In D714#17, @nwhitehorn wrote:

Julian, I agree. That's why I first asked what else this was for. If it's a general purpose tool that adds really useful features and this is the first use, that's absolutely fine. It would be nice to work with upstream to make this a part of libdialog itself, though, rather than wrapping it.
What in general I don't want to happen is that the installer becomes progressively more and more complex with little shared code with the rest of the system. It lives in a really awkward place in the software stack where it absolutely must be bug-free and simultaneously is almost never tested since it's a program that people use once. Introducing crippling regressions here is thus really easy; the only way around that is to be sure that as much code as possible is shared with tools that people do use every day. There's not that much regression risk here, since this is just tar with progress bars and is hard to get wrong in subtle ways, but that's anyway why I'm concerned to see multi-thousand line diffs to a 150-line program.
One note on the code itself. I don't know if this is possible, but it might also be nice to simplify the API a little: interacting with libdpv rather than libdialog nearly doubles the total code length of distextract in this patch.

The claim above that (quote) "interacting with libdpv rather than libdialog nearly doubles the total code length" is patently false.

To prove this, I've made a version of distextract.c that uses dpv which produces a _minimal_ diff when compared against the original.
Not only is the diff minimal, the replacement has fewer lines than the original:

distextract.c.dpv_minimal
distextract.c.dpv_minimal.patch

In truth, interacting with libdpv rather than libdialog REDUCES the total code length.
It just so happened, that the code-length of bsdinstall/distextract/distextract.c is set to grow because I've made the following changes "while I was here" (cleaning up the code that was already there):

  1. Added a copyright entry for myself
  2. Moved $FreeBSD$ from comment to __FBSDID
  3. Added explicit includes
  4. Added whitespace
  5. Added comments
  6. Added interrupt (INT) signal handler
  7. Added #ifdef's to assuage compilation on older libarchive NB: Minimize diff between branches; make merging easier.
  8. Add missing calls to end_dialog(3)
  9. Change string processing from strtok(3) to strcspn(3)
  10. Use EXIT_SUCCESS and EXIT_FAILURE instead of 0/1
  11. Apply style(9) to variable declarations
  12. Added missing function prototypes (e.g., for count_files())
  13. Changed MAXPATHLEN to PATH_MAX
  14. Reset pointers to NULL after free(3)
  15. Initialize pointers to NULL in declaration NB: If there's a chance they could be used before re-initialization
  16. Use snprintf in place of unbounded sprintf
  17. Don't declare variables in if-, while-, or other statement NB: To prevent masking of prior declarations atop function
  18. Prevent race conditions where getenv(3) is used to obtain an environment variable, taint-check it, and then after having decided that it is taint-free re-obtain a new value using getenv(3) a second time (wherein the value might have changed between getenv(3) calls). NB: In some cases, getenv(3) was called yet a third, fourth, etc. time after only having validated the value at the onset of main()
  19. Use strtol(3) instead of [deprecated] atoi(3)
  20. Add additional error checking (e.g., check return of archive_read_new(3))
  21. Use err(3), errx(3), warn(3), and warnx(3) instead of fprintf(3) etc.
  22. Break out single-line multi-declarations NB: To prevent masking of similarly-named variables with one-char difference
  23. Don't use strdup(3) on a getenv(3) variable
  24. Just use the getenv(3) response -- possible because I switched from using strtok(3) to strcspan(3) (read-only parsing)
  25. Assign DECONST strings to static globals for persistent use NB: Not required, but backtitle now used in more than one place
  26. Centralize the [long] list of bitmask flags to pass to archive_read_extract(3)
  27. style(9) sort header includes
  28. Rename 'err' ints to 'rv' or 'retval' to prevent masking 'err' declaration in <err.h>
  29. Required when including <err.h>

I might add, that even with those changes, the break down is _still_ not close to "doubl[ing] the total code length" (see below):

239 distextract.c.dpv_minimal
241 distextract.c.orig
342 distextract.c

Last I checked: $ echo 'scale=5; 342/241' | bc
1.41908

A minimal patch to distextract.c which only transitions from libdialog(3) to dpv(3) causes a reduction, but it is the above 20+ _other_ changes that cause a 41.9% growth (which is 58.1% away from "doubl[ing] the total code length".

I might add that the above list of changes is directly in the spirit of (quote from yourself above) [bsdinstall] absolutely must be bug-free and simultaneously is almost never tested... (/quote). I can tell you that the code in its current state prior to this patch is bug free and very tested.

Devin

dteske added a comment.EditedSep 12 2014, 8:01 PM
In D714#16, @julian wrote:

I'm not sure if I'd hide the lib code inside the context of the installer and move it out when other users materialise, or put it out in the plain firing line in its own directory. I suspect that personally I'd do the first, but still make sure that it was easily understood how to access it from a 2nd application. later if there are more users it could be moved out. Either would be fine for me.

Two port maintainers (and a recent potential third) have contacted me to express interest in using this tool from their port. For example, the maintainer of sysutils/zxfer would like to use it to visualize ZFS snapshot transfers. So I'm in favor of (quote) put[ting] it out in the plain firing line in its own directory.(/quote) I envisage the most meaningful feedback leading to enhancements under my maintainership from it being out in the open.

Devin

dteske added a comment.EditedSep 12 2014, 8:14 PM
In D714#14, @ngie wrote:

I agree with nathanw@ about the complexity. It's best to avoid feature creep in the installer; otherwise bsdinstall will turn into sysinstall.
Please see if you can add kientzle@ to review the libarchive code (he's not a Phabric user, so you'll need to prod him about creating an account).
It seems like distextract could be replaced with a much simpler shell script; if so, would it render the dpv work unnecessary or could it instead use the dpv program [directly/indirectly] to better abstract out the MVC nature of the code you're trying to add?
[this applies to the new code as well as the existing code]
Did you find this thread that kientzle@ responded to: https://groups.google.com/forum/#!topic/libarchive-discuss/0MWyuZafnpM ? It

It was battling feature creep that made me abstract all the logic out to a library.

It's not impossible to code distextract in shell... see here:

http://pastebin.com/NcSNryfL

But if we have dpv, then the above shell script looks more like this:

#!/bin/sh
dpv -m \
	-t "Distributions" \
	-b "FreeBSD Installer" \
	-p "Extracting distributions into / directory..." \
	-a "\n  Overall Progress:" \
	-x "tar zxf - -C /" \
	base.txz /path/to/base.txz \
	ports.txz /path/to/ports.txz \
	games.txz /path/to/games.txz

Much smaller and way more manageable, eh?

Doing a full shell script solution would be even uglier than the above pastebin prototype (which is in-fact just a prototype). Eventually you'd run into limitations that mean hard edge-cases (especially when it comes to pedantic error checking -- which is required in all parts of the installer code). Hence-why dpv.

Devin

I really don't want to argue about code length here, but, for clarity, my line count removed whitespace and comments, which are about half of the original code.

It's still unclear to me what this adds besides a speed measurement, but I'm glad to hear that it's for general use. A few comments on the patch:

  1. It would be nice if the miscellaneous changes you mentioned were not part of a patch to the UI. The usual process is to separate style and functional changes into separate commits.
  2. The comments about style(9) are unnecessary, since code is assumed to be style(9) and style(9) explicitly discourages comments like this. The kernel, for example, mentions the style guide only twice in code, both times in conjunction with explaining why code violates the guidelines.
  3. Moving the flags and a few other things to variables makes the code harder to follow. They are used only once, so there is no reason to move them away from the place they are used.
  4. It would be really good if you could merge this into upstream libdialog. Thomas Dickey is very receptive to patches in general.
  5. We already have a number of config file parsing libraries in base. libucl, for example, was imported by pkg for this purpose. Why libfigpar?
In D714#21, @nwhitehorn wrote:

I really don't want to argue about code length here, but, for clarity, my line count removed whitespace and comments, which are about half of the original code.

Taking said "clarity" into account, you're still wrong:

147 distextract.c.orig.stripped
161 distextract.c.dpv_minimal.stripped (9% larger)
213 distextract.c.stripped (44% larger; fixes race conditions, buffer overflows, etc.)

If you don't want to argue about code length, don't make false statements such as "interacting with libdpv rather than libdialog nearly doubles the total code length of distextract in this patch". Please ask questions before making bold statements that aren't true (my own wife likes to do the same thing... round a number like "1.1" up to "2.0"; it's just wrong; in this case you're rounding from 1.09 up to 2.0 -- see "distextract.c.dpv_minimal" linked-to in comment #18).

In D714#21, @nwhitehorn wrote:

It's still unclear to me what this adds besides a speed measurement, but I'm glad to hear that it's for general use.

Stated in the proposed commit message (here and on the mailing list; more than once).

In D714#21, @nwhitehorn wrote:

A few comments on the patch:

  1. It would be nice if the miscellaneous changes you mentioned were not part of a patch to the UI. The usual process is to separate style and functional changes into separate commits.

What exactly are you talking about?

In D714#21, @nwhitehorn wrote:
  1. The comments about style(9) are unnecessary, since code is assumed to be style(9) and style(9) explicitly discourages comments like this. The kernel, for example, mentions the style guide only twice in code, both times in conjunction with explaining why code violates the guidelines.

Very well, I will gladly take care of that.

In D714#21, @nwhitehorn wrote:
  1. Moving the flags and a few other things to variables makes the code harder to follow. They are used only once, so there is no reason to move them away from the place they are used.

Certainly.

In D714#21, @nwhitehorn wrote:
  1. It would be really good if you could merge this into upstream libdialog. Thomas Dickey is very receptive to patches in general.

I have already worked with Thomas Dickey in the past.

http://svnweb.freebsd.org/base?view=revision&revision=255852
http://invisible-island.net/dialog/CHANGES
NB: Search for "Teske" in dialog's CHANGES file

While I'm certain that he would readily receive dpv, I want to minimize upstream dumps to only a few per year (at the most) to mitigate lengthy waiting periods where we're waiting for the vendor dump to make it back into our tree (it takes time for Thomas Dickey to cut each release; and he's not too fast about it).

Fine-tuning it within our own tree before we make a run at upstream integration (relinquishing control over it) makes sense until the changes subside. Discussing this before the software is even in the tree is somewhat moot; just know that I have more changes beyond the initial offering that I would want integrated prior to the initial upstream offering. Not to mention that the code may not even compile on all the systems that dialog currently targets (read: Thomas has already ran my code and he would make some changes for Linux -- among the least would be to parameterize the declaration of _BSD_SOURCE declared for FreeBSD headers).

So I'm not against upstreaming this; but I am against upstreaming it right now.

In D714#21, @nwhitehorn wrote:
  1. We already have a number of config file parsing libraries in base. libucl, for example, was imported by pkg for this purpose. Why libfigpar?

Just as I have plans to use dpv(1)/dpv(3) in many places, libfigpar is especially crafted. I'm writing a tool called "sysconf" that uses it. But that's for another discussion another time (because I have my doubts that you can dote on anything I do).

GUYS! stoppit! your arguing cross purposes and getting all hot under the collar for no reason!

Devin, After your list of fixes and seeing htat you have a separate diff for it,
I suggest you apply all this in two phases.
Nathan also suggested this but as I said communication is getting hard here..

First. apply all the fixes *without the dpv code*. this is is probably completely non controversial.

Do it now. quote me as reviewer

Then apply the dpv transition. Put it in a separate place outside of the installer and put me on the review list.
Can we get on to more productive things?

That sounds like a good plan to me, Julian. I'd just like to see these things done piece by piece and well thought-through. Aside from the minor nits mentioned, I'm quite happy with the changes.

stas added a subscriber: stas.Nov 5 2014, 2:47 AM
stas added inline comments.
lib/libfigpar/figpar.c
202 ↗(On Diff #1397)

Hmm, shouldn't we do something with the read return value here? It looks like it's ignored and overridden by other read(2) call below.

ngie added a comment.Nov 14 2014, 5:46 PM

This revision should probably be closed..

In D714#23, @julian wrote:

GUYS! stoppit! your arguing cross purposes and getting all hot under the collar for no reason!
Devin, After your list of fixes and seeing htat you have a separate diff for it,
I suggest you apply all this in two phases.
Nathan also suggested this but as I said communication is getting hard here..
First. apply all the fixes *without the dpv code*. this is is probably completely non controversial.
Do it now. quote me as reviewer
Then apply the dpv transition. Put it in a separate place outside of the installer and put me on the review list.
Can we get on to more productive things?

In D714#27, @ngie wrote:

This revision should probably be closed..

Negative; I'm trying to find the cycles to update it -- thus far, I've spun off dpv(1,3)/figpar(3) into a separate commit, BUT... this differential review is for the enhancement of bsdinstall. When I update this with a new patch it will shrink by several thousand lines (and pertain to _only_ the enhancement of bsdinstall).

Actually, now that I think about it, what I'm waiting for is the 21 day MFC that I set to expire so I can merge dpv(1,3)/figpar(3) to stable/{10,9} -- so that I can then set a 3-day MFC on the bsdinstall enhancement herein.

Devin

dteske updated this revision to Diff 2545.Nov 25 2014, 5:51 PM
dteske edited edge metadata.

Update patch to only itemize changes to bsdinstall.
Focus of the review is still "Installer Enhancement"
just that the diff shrinks dramatically since I spun
dpv(1,3)/figpar(3) off into their own commit(s) and
merged back to stable/{9,10}.

This revision now requires review to proceed.Nov 25 2014, 5:51 PM
dteske updated this object.Nov 25 2014, 5:59 PM
dteske edited edge metadata.
dteske accepted this revision.Dec 18 2014, 3:34 AM
dteske added a reviewer: dteske.

ETIMEOUT for re-approve from reviewers

This revision is now accepted and ready to land.Dec 18 2014, 3:34 AM