MFC after: 1 week
Details
Details
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Skipped - Unit
Tests Skipped - Build Status
Buildable 57667 Build 54555: arc lint + arc unit
Event Timeline
| sbin/ifconfig/ifconfig.c | ||
|---|---|---|
| 332 | Would it be worthwhile to generalize, and allow other formats to be combined across multiple address types? The open question would be which address types to include. One option is to assume "addr". Or maybe it's just better to make "addr:cidr" work, and use that in login.conf. btw, it would also be nice if command-line options overrode the environment; they don't seem to at the moment, but that's a change in behavior. | |
| sbin/ifconfig/ifconfig.c | ||
|---|---|---|
| 332 | Le mieux est l'ennemi du bien. | |
| sbin/ifconfig/ifconfig.c | ||
|---|---|---|
| 332 | Perhaps, but inventing a new syntax when there is already one there seems unnecessary. Granted, the "it would be nice" part might fall into that category, but should be a separate change in any case. | |
| sbin/ifconfig/ifconfig.c | ||
|---|---|---|
| 332 | Command-line options already override the environment variable. The latter is handled on lines 629-631 before the call to args_parse() on line 639. | |
Comment Actions
Are there any other reviewers that may be interested? Maybe post a link on the GitHub review?
| sbin/ifconfig/ifconfig.c | ||
|---|---|---|
| 332 | Oh, I see. I expected -f addr:default to override inet:cidr and inet6:cidr in the environment. Now I see that addr: only applies to the address, whereas inet: and inet6: deal with the netmask/prefix length. The man page could be clearer, e.g. by putting the address formats in a separate list from the mask formats. At any rate, my suggestion of addr:cidr doesn't make sense. This makes "default" a bit irregular, applying to both addresses and masks, but I find the current scheme irregular anyway. | |
Comment Actions
Generally i like it. It does seem to mix memory leak / style fixed in with the new functionality.
Otherwise i think this is a good change. Like mike i could quibble around the edges, but it wouldn't make it that much better..
| sbin/ifconfig/ifconfig.c | ||
|---|---|---|
| 319 | This can be a separate commit | |
| 624 | Why remove this? | |
| sbin/ifconfig/ifconfig.c | ||
|---|---|---|
| 624 | It's pointless since these variables are static and therefore already initialized to NULL. | |
| sbin/ifconfig/ifconfig.c | ||
|---|---|---|
| 624 | Sorry, I meant global, not static. | |