Page MenuHomeFreeBSD

geom: consistently strip /dev/ prefix at input
Needs ReviewPublic

Authored by delphij on Apr 18 2022, 4:54 AM.
Tags
None
Referenced Files
F56034005: D34940.diff
Tue, Feb 7, 11:44 PM
Unknown Object (File)
Dec 14 2022, 7:06 PM
Unknown Object (File)
Dec 14 2022, 3:16 AM
Unknown Object (File)
Dec 12 2022, 1:56 AM
Subscribers

Details

Reviewers
cem
mav
emaste
Summary

Previously we are stripping the /dev/ prefix at different places
for GEOM providers which was not consistent and resulted in
code duplication.

Address this by extracting the handling of GEOM device name from
request into a new method (gctl_get_devname) and make all GEOM
providers use it.

Test Plan

manually test behavior after change

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 45220
Build 42108: arc lint + arc unit

Event Timeline

LGTM. My only bikeshed contribution is maybe gctl_get_devnameparam or get_devparam. But I don’t object to the current name.

sys/geom/geom.h
431

Seems like we should be able to eliminate either the definition or the paths include, right?

This revision is now accepted and ready to land.Apr 18 2022, 3:52 PM

I have no objections except some thoughts on function names.

I am only curios why you haven't replaces all the cases, but only those? I see gctl_get_ascii(req, "arg0") in gpart_backup()/gpart_restore()/gpart_bootcode()/etc. I was thinking about differentiation between geom and provider names, but you seems to replaced some of both.

sys/geom/geom.h
433

I am thinking whether g_provider_name() would be more specific.

452

... and respectively here "provider" not "device".

This revision now requires review to proceed.Apr 19 2022, 6:18 AM

Thanks for reviewing! Please take another look, thanks!

sys/geom/concat/g_concat.c
906

Is this safe? I know it's a pre-existing thing, but is different than the others.
Alternatively, can the callers assume this i s always != NULL and we can remove some tests?

I've just noticed this touches only kernel. It would probably be good for consistency do the same in user-space (lib/geom), like gpart_backup()/gpart_restore()/gpart_bootcode()/etc.

In D34940#792579, @mav wrote:

I've just noticed this touches only kernel. It would probably be good for consistency do the same in user-space (lib/geom), like gpart_backup()/gpart_restore()/gpart_bootcode()/etc.

Will take a look there too.

sys/geom/concat/g_concat.c
906

This particular one is safe (the parameters passed in line 887-889; if the parameter was omitted, gctl_get_provider would have returned NULL and we have bailed out early).

delphij added inline comments.
sys/geom/concat/g_concat.c
906

can the callers assume this i s always != NULL and we can remove some tests

No, this is not guaranteed. I see this MPASS more of an invariant (because we already checked that for each parameters provided, there was a GEOM provider, which is stronger than parameter has contents) The assertion here is probably to silent some static analyzers ("hey, for other gctl_get_asciiparam callers you always checked if the return was not NULL, why did you skip here").