Page MenuHomeFreeBSD

stand: stop using varargs for dv_open() implementations
AcceptedPublic

Authored by kevans on Apr 15 2022, 4:37 AM.
Tags
None
Referenced Files
Unknown Object (File)
Dec 14 2022, 2:56 AM
Unknown Object (File)
Dec 2 2022, 12:25 PM

Details

Reviewers
imp
tsoome
manu
Group Reviewers
bhyve
Summary

commit de04d704a98 ("Use the actual struct devdesc at the start [...]")
cleaned up all of the *_devdesc implementations to embed struct devdesc
at the beginning of all of the derivatives, to remove some fragility and
generally clean things up a little bit.

Take it a step further and remove varargs from dv_open(). All of these
can assume that a devdesc, at a minimum, will be passed to them.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 48172
Build 45059: arc lint + arc unit

Event Timeline

This revision is now accepted and ready to land.Apr 15 2022, 6:13 PM
stand/i386/libfirewire/firewire.c
179

we can use __unused there.

stand/libsa/geli/gelidev.c
204

and __unused here?

stand/uboot/uboot_disk.c
177

f is now __unused ?:)

stand/usb/storage/umass_loader.c
125

f is now __unused?

stand/userboot/userboot/host.c
136

also __unused?

stand/userboot/userboot/userboot_disk.c
159

f is now __unused?

This revision now requires review to proceed.Apr 16 2022, 3:05 AM

Hm, it seems, we do not need f in any of those instances? Should we just remove that argument?

stand/common/dev_net.c
120

f is unused

stand/common/md.c
124

f is unused

stand/common/vdisk.c
295

f is unused

stand/efi/libefi/efihttp.c
231

f is unused..

stand/efi/libefi/efipart.c
884

f is unused

stand/i386/libi386/biosdisk.c
840

f is unused...

stand/kboot/hostdisk.c
91

f is __unused

stand/libofw/ofw_disk.c
121

f is __unused

Hm, it seems, we do not need f in any of those instances? Should we just remove that argument?

I'm torn between that and removing the devdesc argument instead. The latter would be more consistent with the other callbacks:

157     int         (*dv_close)(struct open_file *f);                               
158     int         (*dv_ioctl)(struct open_file *f, u_long cmd, void *data);

Since we can derive the devdesc from the open_file, it'd be the most flexible option and consistent with the rest of the interface. We'd have to fix userboot to fake an open_file for a call there, but ubldr was already doing that so there's some precedent.

Remove the devdesc arg to dv_open() entirely. f->f_devdata is always a devdesc
for the initial open(), modulo one instance in userboot that calls dv_open()
manually. Adopt it to the new interface like we do in uboot and just fake a
properly constructed struct open_file to pass in.

This matches the other dv_open() methods which operate on a struct open_file.

I think I love the direction
But it's scary how many routines know about disk_open... Even though it looks like it should be a dv_open routine ,it's not :( ... But I've had too much pain with disk.c and part.c of late, so maybe I'm biased.

stand/common/devopen.c
52

Love this change.

stand/i386/libfirewire/firewire.c
179

Doesn't need the second argument, right?

stand/i386/libi386/biosdisk.c
868

Not for this commit, but all these casts suggest two things to me:

  1. We might want to have a macro to do the conversion.
  2. We really should assert in disk_open that dev->d_dev->dv_type == DEVT_DISK

But maybe these are followup work.

stand/kboot/hostdisk.c
95

this will likely cause me a merge conflict, but this change is worth it.

stand/libofw/ofw_disk.c
125

similar comments about an assert here, which is why a wrapper to get the devdesc from f might be useful to have that assert buried in... But more just idle thoughts than a request for you to change this already longer than ideal size patch

stand/libsa/stand.h
139–140

These are OBE sine we declare this 6 lines later.

stand/usb/storage/umass_loader.c
47

drop second arg here, no?

125

second arg 86?

kevans marked 13 inline comments as done.

Address new comments, drop in some XXX notes for future work

I'd leave out the d_size change at the moment since I'm not 100% sure that that's completely safe yet....

stand/common/dev_net.c
123

might be better to have this first so the variables are in alphabetical order... But this is also super-duper picky and likely doesn't matter if you don't want to change it.

stand/common/disk.c
241

I don't know of any place we create a disk_devdesc for a DEVT_DISK, but this is a good signpost for the future.

stand/libsa/stand.h
183

This snuck in...
And I think it's fine now (I fixed how i386_devdesc was declared to use unions), but since we're not yet using it it would be less risky to omit it.

This revision is now accepted and ready to land.Nov 5 2022, 8:00 PM
stand/i386/libi386/biosdisk.c
868

Good points- dropping in some comments.

stand/libofw/ofw_disk.c
125

The problem with a macro for that in particular is that we can only rely on f->f_devdata being the devdesc during dv_open(). Most drivers won't replace it with their own devdata, but one or two off in efi/ land will. It's probably OK, it just needs a bit of care.

stand/libsa/stand.h
183

Whoops, sorry, I had screwed up on some squashing and this snuck in from the next commit (D34925) while I was fixing it