Page MenuHomeFreeBSD

Add kern.geom.part.separator.
Needs ReviewPublic

Authored by trasz on Wed, Oct 30, 3:14 PM.

Details

Reviewers
None
Group Reviewers
manpages
Summary

Add kern.geom.part.separator tunable. This makes it possible
to specify an optional separator to insert before partition name;
eg if it's set to "c/", you'll get "ada0c/s1" instead of "ada0s1". (It cannot be set to just “/“, since ada0 is a device node, not a directory.)

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 27401
Build 25648: arc lint + arc unit

Event Timeline

trasz created this revision.Wed, Oct 30, 3:14 PM

Is the 'd' or 'c' in the description a typo? Should it be ada0d/s1? or ada0/s1

imp added a comment.Wed, Oct 30, 5:57 PM

There are lots of places in the system that will break when this is not '' . What are your plans on fixing those?

trasz edited the summary of this revision. (Show Details)Wed, Oct 30, 6:34 PM

Is the 'd' or 'c' in the description a typo? Should it be ada0d/s1? or ada0/s1

Yup, just a typo; fixed.

trasz added a comment.Wed, Oct 30, 6:37 PM
In D22193#485062, @imp wrote:

There are lots of places in the system that will break when this is not '' . What are your plans on fixing those?

It’s empty by default; if user needs it I suppose it’s their responsibility to update paths in eg fstab. Or do you mean there are some scripts/binaries that hardcode it?

imp added a comment.Wed, Oct 30, 6:40 PM
In D22193#485062, @imp wrote:

There are lots of places in the system that will break when this is not '' . What are your plans on fixing those?

It’s empty by default; if user needs it I suppose it’s their responsibility to update paths in eg fstab. Or do you mean there are some scripts/binaries that hardcode it?

I mean there's at least a dozen places that assume they can construct a path to a slice from a path to the dev and don't know about this. It's better than it used to be, but I'll bet there's still a lot of breakage.

imp added a comment.Wed, Oct 30, 6:44 PM
/*
  * Fill in the devid, now that we've labeled the disk.
  */
 (void) snprintf(buf, sizeof (buf), "%ss%d", path, slice);
 if ((fd = open(buf, O_RDONLY)) < 0) {
         (void) fprintf(stderr,
             gettext("cannot open '%s': %s\n"),
             buf, strerror(errno));
         return (-1);
 }

is one example, this one is from cddl/contrib/opensolaris/cmd/zpool/zpool_vdev.c

imp added a comment.Wed, Oct 30, 6:46 PM

There's likely others, but I've not grepped them up yet.

trasz updated this revision to Diff 63823.Thu, Oct 31, 11:05 AM

Update the man page.

trasz added a comment.Thu, Oct 31, 5:58 PM
In D22193#485070, @imp wrote:
/*
  * Fill in the devid, now that we've labeled the disk.
  */
 (void) snprintf(buf, sizeof (buf), "%ss%d", path, slice);
 if ((fd = open(buf, O_RDONLY)) < 0) {
         (void) fprintf(stderr,
             gettext("cannot open '%s': %s\n"),
             buf, strerror(errno));
         return (-1);
 }

is one example, this one is from cddl/contrib/opensolaris/cmd/zpool/zpool_vdev.c

It seems to be under '#ifdef illumos', though. FWIW, "zpool create", "zpool import" etc seem to work just fine with separator set to "c/".

In D22193#485070, @imp wrote:
/*
  * Fill in the devid, now that we've labeled the disk.
  */
 (void) snprintf(buf, sizeof (buf), "%ss%d", path, slice);
 if ((fd = open(buf, O_RDONLY)) < 0) {
         (void) fprintf(stderr,
             gettext("cannot open '%s': %s\n"),
             buf, strerror(errno));
         return (-1);
 }

is one example, this one is from cddl/contrib/opensolaris/cmd/zpool/zpool_vdev.c

We should try to find these cases and clean them up. I wouldn't expect there to be too many, since everything should be using GEOM to create partitions etc.

trasz added a comment.Fri, Nov 1, 11:36 PM
In D22193#485070, @imp wrote:
/*
  * Fill in the devid, now that we've labeled the disk.
  */
 (void) snprintf(buf, sizeof (buf), "%ss%d", path, slice);
 if ((fd = open(buf, O_RDONLY)) < 0) {
         (void) fprintf(stderr,
             gettext("cannot open '%s': %s\n"),
             buf, strerror(errno));
         return (-1);
 }

is one example, this one is from cddl/contrib/opensolaris/cmd/zpool/zpool_vdev.c

We should try to find these cases and clean them up. I wouldn't expect there to be too many, since everything should be using GEOM to create partitions etc.

But how to find them?

brueffer added inline comments.
lib/geom/part/gpart.8
1264

"that will be inserted between GEOM name and partition name", is that what you mean?

allanjude added inline comments.Fri, Nov 8, 7:06 PM
lib/geom/part/gpart.8
1264

Yes, that is what is meant here.

The later sentence can be improved as well:

"Note that setting this variable may break software which assumes a particular naming scheme."

trasz updated this revision to Diff 64088.Fri, Nov 8, 7:33 PM

Feedback from brueffer@ and allanjude@.

trasz marked 2 inline comments as done.Fri, Nov 8, 7:33 PM

FWIW, grepping for that pattern only brings up the one hit Warner has already mentioned, and it's already under #ifdef illumos:

% grep -r '%ss%d' *
cddl/contrib/opensolaris/cmd/zpool/zpool_vdev.c:                (void) snprintf(buf, sizeof (buf), "%ss%d", path, slice);