Page MenuHomeFreeBSD
Differential D14978

Refactor PCIOCGETCONF for improved readability.
ClosedPublic
Actions

Authored by brooks on Apr 5 2018, 6:47 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Oct 26, 12:45 AM
Unknown Object (File)
Oct 10 2024, 12:26 AM
Unknown Object (File)
Oct 2 2024, 9:21 PM
Unknown Object (File)
Sep 23 2024, 5:31 AM
Unknown Object (File)
Sep 21 2024, 11:18 AM
Unknown Object (File)
Sep 20 2024, 1:46 AM
Unknown Object (File)
Sep 16 2024, 6:17 PM
Unknown Object (File)
Sep 8 2024, 9:56 PM
View All 53 Files
Subscribers
imp

Details

Reviewers
kib
emaste
jhb
Commits
rS332343: Refactor PCIOCGETCONF for improved readability.
Summary

The code now has a single, consistant flow for all three ioctl
variants. ifdefs and for pre-FreeBSD-7 compatability are moved to
functions and macros. So the flow is alwasy the same, we impose
the cost of allocating, copying to, updating from, and freeing a
copy of struct pci_conf_io on all paths.

This change will allow PCIOCGETCONF32 support currently in
sys/compat/freebsd32/freebsd32_ioctl.c to be moved here.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

brooks created this revision.Apr 5 2018, 6:47 PM
Harbormaster completed remote builds in B15995: Diff 41143.Apr 5 2018, 6:47 PM
brooks updated this revision to Diff 41159.Apr 5 2018, 10:49 PM
  • Correct a leftover from a botched substitution.
Harbormaster completed remote builds in B16004: Diff 41159.Apr 5 2018, 10:49 PM
brooks updated this revision to Diff 41161.Apr 5 2018, 10:57 PM
  • tab's after #define
  • Be a little more consistant with #ifdefs
Harbormaster completed remote builds in B16006: Diff 41161.Apr 5 2018, 10:58 PM
brooks added reviewers: kib, emaste, jhb.Apr 5 2018, 11:00 PM

I think this is ready for review now.

kib accepted this revision.Apr 6 2018, 12:00 PM
This revision is now accepted and ready to land.Apr 6 2018, 12:00 PM
jhb added a comment.Apr 6 2018, 6:45 PM

One other possibility is to define a helper struct that contains the structure sizes and has function pointer callbacks (kevent and aio took this approach), but this is probably fine.

jhb added inline comments.Apr 6 2018, 6:47 PM
sys/dev/pci/pci_user.c
418 ↗(On Diff #41161)

While I appreciate the desire to reduce duplication, this feels a bit squishy to me, especially given it's only used twice. I think the expanded version in the two places might be less total lines than this set of helper macros. I don't think we use this pattern anywhere else in the tree.

brooks added a comment.Apr 6 2018, 8:29 PM

As far as the ops structures, I found the structure in aio frustrating when I needed to change its interface due to having to (e.g.) add N new functions and add the right ones to N+2 or so ops struct instances.

sys/dev/pci/pci_user.c
418 ↗(On Diff #41161)

If makes more sense as the number of entries grows. I've got a patch which adds another case for PCIOCGETCONF32 (moving it out of sys/compat/freebsd32). On CheriBSD I've got yet another PCIOCGETCONF_C. I don't feel strongly about this case, but the virtue of this is that pci_ioctl() now contains no references to compat defines.

jhb added inline comments.Apr 6 2018, 9:15 PM
sys/dev/pci/pci_user.c
418 ↗(On Diff #41161)

I suppose static analysis expands macros so wouldn't be confused, but I wonder if things like clang-format or auto-indenting in emacs/vim handles this syntax ok? Maybe the trailing colon is good enough for that to not break?

brooks updated this revision to Diff 41270.Apr 8 2018, 4:30 PM
  • Expand CASE_PCIOCGETCONF inplace and remove the defintion.
This revision now requires review to proceed.Apr 8 2018, 4:30 PM
Harbormaster completed remote builds in B16066: Diff 41270.Apr 8 2018, 4:30 PM
jhb accepted this revision.Apr 9 2018, 10:27 PM
This revision is now accepted and ready to land.Apr 9 2018, 10:27 PM
Closed by commit rS332343: Refactor PCIOCGETCONF for improved readability. (authored by brooks). · Explain WhyApr 9 2018, 10:59 PM
This revision was automatically updated to reflect the committed changes.
Herald added a subscriber: imp. · View Herald TranscriptApr 9 2018, 10:59 PM

Revision Contents
Changeset List

PathSize
head/
sys/
dev/
pci/
433 lines

Diff 41310

View Options

head/sys/dev/pci/pci_user.c

Loading...