Page MenuHomeFreeBSD
Differential D17116

Add "geom -p".
ClosedPublic
Actions

Authored by trasz on Sep 11 2018, 2:50 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Dec 3, 6:51 AM
Unknown Object (File)
Mon, Dec 2, 9:42 AM
Unknown Object (File)
Mon, Dec 2, 9:42 AM
Unknown Object (File)
Mon, Dec 2, 9:42 AM
Unknown Object (File)
Mon, Dec 2, 9:42 AM
Unknown Object (File)
Mon, Dec 2, 1:10 AM
Unknown Object (File)
Mon, Dec 2, 1:04 AM
Unknown Object (File)
Mon, Dec 2, 1:04 AM
View All 96 Files
Subscribers
0mp
bcr
imp

Details

Reviewers
oshogbo
0mp
Group Reviewers
manpages
Commits
rS338640: Add new option to the geom(8) utility, "-p". It makes it easy to look up
Summary

Add new option to geom(8) utility, "-p". It makes it easy to look up GEOM class instance from the provider name.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 19538
Build 19125: arc lint + arc unit

Event Timeline

trasz created this revision.Sep 11 2018, 2:50 PM
Herald added a reviewer: manpages. · View Herald TranscriptSep 11 2018, 2:50 PM
Herald added a subscriber: imp. · View Herald Transcript
Harbormaster completed remote builds in B19518: Diff 47908.Sep 11 2018, 2:50 PM
trasz updated this revision to Diff 47917.Sep 11 2018, 5:07 PM

Add -t.

Harbormaster completed remote builds in B19524: Diff 47917.Sep 11 2018, 5:07 PM
trasz retitled this revision from Add "geom -P". to Add "geom -p" and "geom -t"..Sep 11 2018, 5:08 PM
trasz edited the summary of this revision. (Show Details)
trasz added a reviewer: oshogbo.
oshogbo requested changes to this revision.Sep 11 2018, 5:37 PM

I proposed some minor changes.

sbin/geom/core/geom.8
27

Don't forgot about it :)

sbin/geom/core/geom.c
667

Should be return (gp);

679

Incorrect according to the style(9).

684

Do we want to handle negative numbers explicit here?

685

Do we need to strdup it?

694

Over 80.

696

Over 80, should be:

show_tree_geom(mesh, gp2,
     indent + 2);
704

Missing dot at the end of the sentence.
In other cases when errx is used the dot is present.

714

This whole part is only when gerprogname is equals to 'geom' maybe is worth doing something like geom_main?

715

Over 80.

833

Missing dot at the end of the sentence.
In other cases when errx is used the dot is present.

This revision now requires changes to proceed.Sep 11 2018, 5:37 PM
0mp requested changes to this revision.Sep 11 2018, 7:24 PM
0mp added a subscriber: 0mp.

Also, are those standard options for valid for any *standard command*? If so then the synopsis section should look a little bit different since at the moment it suggests that the only valid uses of -p and -t are geom -t and geom -p provider-name.

sbin/geom/core/geom.8
110

Is there a reason why it's -width ".Cm status" instead of -width "-p"?

111

Did you mean .It Fl p Ar provider-name? The -p option expects an argument, right?

trasz updated this revision to Diff 47946.Sep 12 2018, 11:37 AM
trasz edited the summary of this revision. (Show Details)

Fix issues and drop -t for the moment.

Harbormaster completed remote builds in B19537: Diff 47946.Sep 12 2018, 11:37 AM
trasz marked 4 inline comments as done.Sep 12 2018, 11:40 AM
trasz added inline comments.
sbin/geom/core/geom.8
110

It's to keep it aligned with the .Bl above.

sbin/geom/core/geom.c
684

Let's leave the "tree" part for another review, it needs more work.

685

I never remember. It seems to be a pretty common pattern.

714

Nah, it's just a few lines of code. Besides it would result in a rather weird flow, since we would call geom_main() and in some cases it would return, and in others it wouldn't.

trasz updated this revision to Diff 47947.Sep 12 2018, 11:41 AM

Style.

Harbormaster completed remote builds in B19538: Diff 47947.Sep 12 2018, 11:41 AM
trasz marked an inline comment as done.Sep 12 2018, 11:42 AM
trasz retitled this revision from Add "geom -p" and "geom -t". to Add "geom -p"..
trasz edited the summary of this revision. (Show Details)
trasz updated this revision to Diff 47948.Sep 12 2018, 11:43 AM

Fix .Dd.

Harbormaster completed remote builds in B19539: Diff 47948.Sep 12 2018, 11:43 AM
trasz added a comment.Sep 12 2018, 11:45 AM
In D17116#365084, @0mp wrote:

Also, are those standard options for valid for any *standard command*? If so then the synopsis section should look a little bit different since at the moment it suggests that the only valid uses of -p and -t are geom -t and geom -p provider-name.

Those options are only to be used with geom(8), not gwhatever(8) commands nor the "standard commands" like the "list" or "load".

0mp accepted this revision as: 0mp.Sep 12 2018, 4:23 PM

The manpage update looks fine.

sbin/geom/core/geom.8
110

+1

bcr added a subscriber: bcr.Sep 13 2018, 7:40 AM

Yes, manpage update approved by mentor, although this is not needed with a src bit. ;-)

sbin/geom/core/geom.8
27

You are the wind beneath my wings, @oshogbo. ;-)

oshogbo accepted this revision.Sep 13 2018, 9:08 AM

One minor change. Besides that LGTM.

sbin/geom/core/geom.8
27

xD

sbin/geom/core/geom.c
702

According to style(9) only boolean can be used like that.
Should be:
if (provider_name != NULL)

This revision was not accepted when it landed; it landed in state Needs Review.Sep 13 2018, 2:06 PM
Closed by commit rS338640: Add new option to the geom(8) utility, "-p". It makes it easy to look up (authored by trasz). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
sbin/
geom/
core/
12 lines
69 lines

Diff 47947

View Options

sbin/geom/core/geom.8

Loading...
View Options

sbin/geom/core/geom.c

Loading...