Page MenuHomeFreeBSD

Distinguish between "no partition" and "choose best partition" with a constant.
ClosedPublic

Authored by ian on Feb 20 2019, 3:22 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Mar 11, 7:25 AM
Unknown Object (File)
Mon, Mar 11, 7:25 AM
Unknown Object (File)
Mon, Mar 11, 7:25 AM
Unknown Object (File)
Mon, Mar 11, 7:25 AM
Unknown Object (File)
Mon, Mar 11, 7:25 AM
Unknown Object (File)
Thu, Mar 7, 10:37 PM
Unknown Object (File)
Feb 24 2024, 9:10 PM
Unknown Object (File)
Feb 9 2024, 10:56 PM
Subscribers

Details

Summary

The values of the d_slice and d_partition fields of a disk_devdesc have a few values with special meanings in the disk_open() routine. Through various evoluations of the loader code over time, a d_partition value of -1 has meant both "use the first ufs partition found in the bsd label" and "don't open a bsd partition at all, open the raw slice."

This defines a new special value of -2 to mean open the raw slice, and it gives symbolic names to all the special values used in d_slice and d_parition, and adjusts all existing uses of those fields to use the new constants.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 22601

Event Timeline

Update comments to explain D_PARTWILD and use D_PARTISGPT instead of 255.

stand/common/disk.c
137

We did disk_open() and should be using handle from it. pa->dev is working here only because we *usually* do not have second level of nesting and we only do use sectorsize from this structure.

239

Actually this whole partdev variable is not needed. We only need to save the original slice and partition values (which we do anyhow).

On successful disk_open() we end up dev having the correct slice and partition set, and d_offset pointing to either disk or partition/slice start.

266

There and at line 277 we still continue to use the original hack and we do not check for the D_PARTWILD.

Perhaps something like this will give better overview what we want to see and will avoid getting lost in the if statement spaghetti:

switch (ptable_gettype(od->table)) {
case PTABLE_FREEBSD:

 if (partition == D_PARTWILD)
     ..
else
     ..
break;

case PTABLE_ISO9660:

...
break;

case PTABLE_GPT;

...
break;

case PTABLE_MBR:

...

}

369

Such constructs are now bad; it depends on specific value setup and is assuming particular ordering. If someone will change the values, all the related code will suddenly be wrong.

395

D_SLICENONE

stand/libsa/zfs/zfs.c
591

D_SLICENONE?

stand/uboot/common/main.c
342

Same as above about carefully crafted value ordering...

I love this. Or at least the idea of this.
If we also kill some of the type-punning that remains for disk_devdesc and other types, I think we'll have tamed the beast
I've not checked through every single instance as I'm busy today, but wanted to blast out that I loved this.
The only other alternative might be to have a flags field inside of disc_devdesc which controls this sort of behavior if that would be easier. Given how pervasive this idiom is, though, I'm guessing maybe not. I mention this in case I'm wrong in my guess.

stand/common/disk.c
266

I specifically didn't change anything in disk_open() because I don't want to change the logic and change all the names at the same time. I'd rather commit the number->name changes and then commit a rewrite of disk_open() along the lines you suggest here in a week or so as a separate thing. That way if something goes wrong we don't have to guess whether it was a bad number->name change somewhere or bad logic in the disk_open() rewrite.

369

Same as disk_open(), I'd rather leave the logic changes for the next round.

395

Good catch, I missed this one somehow. But, it looks like what we actually want is D_SLICEWILD, which is a behavior change, but a sensible one. I would expect "ls disk0:" to work and find the best slice and partition, which it will only do with D_SLICEWILD. This is the one place where I do think we should change the logic by using WILD instead of NONE.

stand/uboot/common/main.c
342

This one is less complicated than disk_open(), so I will go ahead and switch to using the names here even though it's a bit of a logic change.

Fix a couple missed number->name changes.

In disk_parsedev() the change of slice = -1 to slice = D_SLICEWILD is actually a logic change as well, allowing diskN: to find the right slice and partition using the usual rules (preferring active freebsd partitions, etc).

This revision was not accepted when it landed; it landed in state Needs Review.Mar 24 2019, 6:52 PM
This revision was automatically updated to reflect the committed changes.
ian marked an inline comment as done and 2 inline comments as not done.