loader: disk api should honor d_partition = -1 and open slice

[tsoome]

Summary

When disk_open() is called with d_partition set to -1, we should open slice
and not the first partition from that slice.

Test Plan

use lsdev with BSD partition table inside the MBR slice, open
files from such partition.

[imp]

Can we please spell -1 "SLICE_ONLY" or "Full slice" or something similar? I hate -1 being a special case that I have to puzzle out for each instance of it we have in the code.

[• ian]

This is insufficient. Whether we like it or not, whether it's documented or not, it is an existing feature that disk_open() with partition set to any negative number means "use the first freebsd-ufs partition in the slice". This isn't documented anywhere, even the loader(8) manpage only says "the syntax for devices is odd". But if we change the current behavior, peoples' existing configurations can break, leaving them without access to a remote system, etc.

Ubldr very explicitly relies on (and documents, at least in comments) that partition == -1 means probe for a good partition. It looks to me like the code in biosdisk.c for x86 also implicitly assumes and expects that (through a really twisty path that ultimately traces back to disk_parsedev() setting d_partition to -1 and leaving it that way if there is no pX or sX on the end.

It does look like the negative number aspect is univserally -1, so we should be free to define that with a name that means 'give me what you got' and then define a new -2 or other value to mean 'give me the raw slice'.

BTW, fwiw, this isn't a problem for slices because -1 means "not initialized", 0 means "raw slice" and 1+ are slice numbers. It appears disk_open() with a negative slice number probably falls on its face, returning success with d_slice set to the negative number, but that's based on running the code in my head, haven't tested on a real machine.

[tsoome]

In D19238#411581, @ian wrote:

This is insufficient. Whether we like it or not, whether it's documented or not, it is an existing feature that disk_open() with partition set to any negative number means "use the first freebsd-ufs partition in the slice". This isn't documented anywhere, even the loader(8) manpage only says "the syntax for devices is odd". But if we change the current behavior, peoples' existing configurations can break, leaving them without access to a remote system, etc.

Ubldr very explicitly relies on (and documents, at least in comments) that partition == -1 means probe for a good partition. It looks to me like the code in biosdisk.c for x86 also implicitly assumes and expects that (through a really twisty path that ultimately traces back to disk_parsedev() setting d_partition to -1 and leaving it that way if there is no pX or sX on the end.

It does look like the negative number aspect is univserally -1, so we should be free to define that with a name that means 'give me what you got' and then define a new -2 or other value to mean 'give me the raw slice'.

BTW, fwiw, this isn't a problem for slices because -1 means "not initialized", 0 means "raw slice" and 1+ are slice numbers. It appears disk_open() with a negative slice number probably falls on its face, returning success with d_slice set to the negative number, but that's based on running the code in my head, haven't tested on a real machine.

Well, you have no place in config to state that the slice or partition is to be set -1. However, to preserve the working configs (where disk*: strings are set, what needs to be done is to update archsw.arch_getdev() to check and add partition a if needed (that is, if we have MBR freebsd slice and BSD label in it). But that can and should be separate patch.

[• ian]

In D19238#411584, @tsoome wrote:

Well, you have no place in config to state that the slice or partition is to be set -1. However, to preserve the working configs (where disk*: strings are set, what needs to be done is to update archsw.arch_getdev() to check and add partition a if needed (that is, if we have MBR freebsd slice and BSD label in it). But that can and should be separate patch.

If the device string is just disk0s1: then the partition is initialized to -1 in disk_parsedev(), which means on all arches leaving off the partition on a BSD slice means (and has always meant) "use the first freebsd-ufs partition in the slice".

[tsoome]

In D19238#411585, @ian wrote:

In D19238#411584, @tsoome wrote:

Well, you have no place in config to state that the slice or partition is to be set -1. However, to preserve the working configs (where disk*: strings are set, what needs to be done is to update archsw.arch_getdev() to check and add partition a if needed (that is, if we have MBR freebsd slice and BSD label in it). But that can and should be separate patch.

If the device string is just disk0s1: then the partition is initialized to -1 in disk_parsedev(), which means on all arches leaving off the partition on a BSD slice means (and has always meant) "use the first freebsd-ufs partition in the slice".

Which means we do have the place for fix. And it has to be fixed where we do translate string descriptor to device structure. If we can add 'a' partition or not can be easily checked by simple disk_open() call. As simple as it.

[tsoome]

In D19238#411586, @tsoome wrote:

In D19238#411585, @ian wrote:

In D19238#411584, @tsoome wrote:

Well, you have no place in config to state that the slice or partition is to be set -1. However, to preserve the working configs (where disk*: strings are set, what needs to be done is to update archsw.arch_getdev() to check and add partition a if needed (that is, if we have MBR freebsd slice and BSD label in it). But that can and should be separate patch.

If the device string is just disk0s1: then the partition is initialized to -1 in disk_parsedev(), which means on all arches leaving off the partition on a BSD slice means (and has always meant) "use the first freebsd-ufs partition in the slice".

Which means we do have the place for fix. And it has to be fixed where we do translate string descriptor to device structure. If we can add 'a' partition or not can be easily checked by simple disk_open() call. As simple as it.

But of course things are not just as simple as they seem to - in disk_parsedev() we do not have enough information to actually call disk_open(), but we do have it in devopen() and that is the place:)

Change is presented in D19243.