Skip to content

Follow-ups to #22587 and #23269#23712

Closed
topimiettinen wants to merge 1 commit intosystemd:mainfrom
topimiettinen:follow-ups-to-22587-and-23269
Closed

Follow-ups to #22587 and #23269#23712
topimiettinen wants to merge 1 commit intosystemd:mainfrom
topimiettinen:follow-ups-to-22587-and-23269

Conversation

@topimiettinen
Copy link
Copy Markdown
Contributor

Add links to documentation to NetLabel, LSM, Netlabel Fallback Peer Labeling
and NFT.

Use a positive boolean function nft_identifier_good() to help grok the checks
more easily than nft_identifier_bad().

Escape logged invalid items.

Add an ASSERT_PTR() for 'data' in config_parse_netlabel().

Use a different debug message when NFT operation fails.

Align nfproto_table strings.

Remove NFT_SET_MSGS, it's only used once.

Add links to documentation to NetLabel, LSM, Netlabel Fallback Peer Labeling
and NFT.

Use a positive boolean function nft_identifier_good() to help grok the checks
more easily than nft_identifier_bad().

Escape logged invalid items.

Add an ASSERT_PTR() for 'data' in config_parse_netlabel().

Use a different debug message when NFT operation fails.

Align nfproto_table strings.

Remove NFT_SET_MSGS, it's only used once.
@topimiettinen
Copy link
Copy Markdown
Contributor Author

focal-amd64 fails with

upstream             FAIL non-zero exit status 1

Probably not related since other focals and everything else pass the tests.

Copy link
Copy Markdown
Member

@yuwata yuwata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do not squash changes. At least, please split the changes into two: for NFT and for NetLabel.


if (nft_identifier_bad(table))
if (!nft_identifier_good(table))
return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "Invalid NFT table name %s.", table);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also escape the string in error message.


if (nft_identifier_bad(set))
if (!nft_identifier_good(set))
return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "Invalid NFT set name %s.", set);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also here.


if (nft_identifier_bad(table))
if (!nft_identifier_good(table))
return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "Invalid NFT table name %s.", table);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also here.


if (nft_identifier_bad(set))
if (!nft_identifier_good(set))
return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "Invalid NFT set name %s.", set);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also here.

const char *rvalue,
void *data,
void *userdata) {
Set **set = ASSERT_PTR(data);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please insert a empty line between the function prototype and variable declaration.

Also, you can drop assert(set); below.

_cleanup_free_ char *esc = NULL;

esc = cescape(table);
return log_syntax(unit, LOG_WARNING, filename, line, 0, "Invalid table name %s, ignoring", esc);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cescape() may fail. Please wrap esc with strna().

_cleanup_free_ char *esc = NULL;

esc = cescape(set);
return log_syntax(unit, LOG_WARNING, filename, line, 0, "Invalid set name %s, ignoring", esc);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also here.

esc = cescape(w);
log_syntax(unit, LOG_WARNING, filename, line, 0,
"Bad NetLabel label, ignoring: %s", w);
"Bad NetLabel label, ignoring: %s", esc);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also here.

if (nft_identifier_bad(set))
return log_syntax(unit, LOG_WARNING, filename, line, 0, "Invalid set name %s, ignoring", set);
esc = cescape(table);
return log_syntax(unit, LOG_WARNING, filename, line, 0, "Invalid table name %s, ignoring", esc);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also here.

_cleanup_free_ char *esc = NULL;

esc = cescape(set);
return log_syntax(unit, LOG_WARNING, filename, line, 0, "Invalid set name %s, ignoring", esc);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also here.

@yuwata yuwata added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks and removed ci-failure-appears-unrelated please-review labels Jun 14, 2022
@yuwata
Copy link
Copy Markdown
Member

yuwata commented Jun 14, 2022

Now I am preparing a PR based on this. Please wait for a while.

@yuwata
Copy link
Copy Markdown
Member

yuwata commented Jun 24, 2022

Sorry, #22587 and #23269 are reverted. Please resubmit them with this change. Closing.

@yuwata yuwata closed this Jun 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cgroups needs-rebase network pid1 reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks

Development

Successfully merging this pull request may close these issues.

3 participants