Add kern.geom.part.separator.

[trasz]

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

[allanjude]

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

[imp]

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

[trasz]

In D22193#485045, @allanjude wrote:

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

Yup, just a typo; fixed.

[trasz]

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]

In D22193#485068, @trasz wrote:

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]

/*
  * 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]

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

[trasz]

Update the man page.

[trasz]

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/".

[allanjude]

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]

In D22193#485541, @allanjude wrote:

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]

lib/geom/part/gpart.8

1264

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

[allanjude]

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]

Feedback from brueffer@ and allanjude@.

[trasz]

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

[allanjude]

In D22193#488245, @trasz wrote:

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]

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

[imp]

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

[trasz]

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