Page MenuHomeFreeBSD

Add kern.geom.part.separator.
ClosedPublic

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

Details

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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

trasz created this revision.Oct 30 2019, 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.Oct 30 2019, 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)Oct 30 2019, 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.Oct 30 2019, 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.Oct 30 2019, 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.Oct 30 2019, 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.Oct 30 2019, 6:46 PM

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

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

Update the man page.

trasz added a comment.Oct 31 2019, 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.Nov 1 2019, 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 ↗(On Diff #63823)

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

allanjude added inline comments.Nov 8 2019, 7:06 PM
lib/geom/part/gpart.8
1264 ↗(On Diff #63823)

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.Nov 8 2019, 7:33 PM

Feedback from brueffer@ and allanjude@.

trasz marked 2 inline comments as done.Nov 8 2019, 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);

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);

%sp%d only found 2 lines in cxgbe, but those are not related to disks.

trasz added a comment.Dec 12 2019, 1:12 PM

Warner, what do you think? Grepping for %ss%d doesn't show any other chunks.

imp accepted this revision.Dec 12 2019, 3:46 PM

So long as you commit to fixing any bugs we find later, I think we're OK.
The only other code that might be an issue is the part editor. I think it might do the right thing, but if you have the means to test it with your change, that would be great

This revision is now accepted and ready to land.Dec 12 2019, 3:46 PM
trasz added a comment.Dec 13 2019, 9:28 AM

Thanks. I'll try to take a look at the editor.

This revision was automatically updated to reflect the committed changes.