Page MenuHomeFreeBSD

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

Authored by ian on Feb 20 2019, 3:22 AM.

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

Event Timeline

ian created this revision.Feb 20 2019, 3:22 AM
ian updated this revision to Diff 54117.Feb 20 2019, 3:38 AM

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

tsoome added inline comments.Feb 20 2019, 6:35 AM
stand/common/disk.c
137 ↗(On Diff #54117)

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 ↗(On Diff #54117)

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 ↗(On Diff #54117)

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 ↗(On Diff #54117)

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 ↗(On Diff #54117)

D_SLICENONE

stand/libsa/zfs/zfs.c
591 ↗(On Diff #54117)

D_SLICENONE?

stand/uboot/common/main.c
342 ↗(On Diff #54117)

Same as above about carefully crafted value ordering...

imp added a comment.Feb 20 2019, 9:44 PM

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.

ian added inline comments.Feb 24 2019, 6:26 PM
stand/common/disk.c
266 ↗(On Diff #54117)

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 ↗(On Diff #54117)

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

395 ↗(On Diff #54117)

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 ↗(On Diff #54117)

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.

ian updated this revision to Diff 54306.Feb 24 2019, 6:42 PM

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

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