Skip to content

Fix default -m delimiter for note ACL #2410

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ankor2023 wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Fix default -m delimiter for note ACL #2410

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
56a3f0e
refactor: hide 'OptionValue::value' variable
ankor2023 Apr 22, 2026
4eccfe4
Update src/acl/Options.h
ankor2023 Apr 28, 2026
448c106
Update src/acl/Options.h
ankor2023 Apr 28, 2026
70adcf9
Addressed reviewer comments
ankor2023 Apr 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/acl/AnnotateClient.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,12 @@ Acl::AnnotateClientCheck::match(ACLChecklist * const ch)
const auto conn = checklist->conn();

if (conn) {
tdata->annotate(conn->notes(), &delimiters.value, checklist->al);
tdata->annotate(conn->notes(), delimiters.value(), checklist->al);
annotated = true;
}

if (const auto &request = checklist->request) {
tdata->annotate(request->notes(), &delimiters.value, checklist->al);
tdata->annotate(request->notes(), delimiters.value(), checklist->al);
annotated = true;
} else if (conn && !conn->pipeline.empty()) {
debugs(28, DBG_IMPORTANT, "ERROR: Squid BUG: " << name << " ACL is used in context with " <<
Expand Down
2 changes: 1 addition & 1 deletion src/acl/AnnotateTransaction.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ Acl::AnnotateTransactionCheck::match(ACLChecklist * const ch)
if (const auto request = checklist->request) {
const auto tdata = dynamic_cast<ACLAnnotationData*>(data.get());
assert(tdata);
tdata->annotate(request->notes(), &delimiters.value, checklist->al);
tdata->annotate(request->notes(), delimiters.value(), checklist->al);
} else {
debugs(28, DBG_IMPORTANT, "WARNING: " << name << " ACL is used in context without " <<
"current transaction information. Did not annotate.");
Expand Down
5 changes: 3 additions & 2 deletions src/acl/CharacterSetOption.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,15 @@ void
TypedOption<CharacterSetOptionValue>::import(const SBuf &rawValue) const
{
SBuf chars = rawValue; // because c_str() is not constant
recipient_->value = CharacterSet(__FILE__, chars.c_str());
recipient_->setValue(CharacterSet(__FILE__, chars.c_str()));
}

template <>
inline void
TypedOption<CharacterSetOptionValue>::printValue(std::ostream &os) const
{
recipient_->value.printChars(os); // TODO: Quote if needed.
if( recipient_->value() ) recipient_->value()->printChars(os); // TODO: Quote if needed.
else os << "[Not set]";

@rousskov rousskov Apr 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If you are adding this if statement "just in case", please do not add it. It is the caller's responsibility to check that the value exists as it is not possible to print a non-existing value correctly. This method can (and probably should) assert (using Assure()) that the value exists.

Otherwise, please discuss what caller(s) prompted you to add this if statement.

@ankor2023 ankor2023 Apr 28, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@rousskov

If you are adding this if statement "just in case", please do not add it. It is the caller's responsibility to check that the value exists as it is not possible to print a non-existing value correctly.

I assumed this method could be used for debugging purposes. In this case, as I understand it, the calling procedure might not know whether the internal variable has been set.

This method can (and probably should) assert (using Assure()) that the value exists.

Refactor it to the following, then?

template <>
inline void
TypedOption<CharacterSetOptionValue>::printValue(std::ostream &os) const
{
    Assure(recipient_->value());
    recipient_->value()->printChars(os); // TODO: Quote if needed.
}

@rousskov rousskov Apr 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I assumed this method could be used for debugging purposes.

I do not think that is a valid assumption in this case. Debugging code should use higher-level methods that handle non-existent values correctly.

Refactor it to the following, then?

If you really insist. FWIW, my recommendation for this method in this PR is to just add () to get this code to compile. This PR is not about improving other aspects of this method, and the proposed assertion is not related to the primary PR changes.

@rousskov rousskov Apr 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Refactor it to the following, then?

If you really insist. FWIW, my recommendation for this method in this PR is to just add () to get this code to compile. This PR is not about improving other aspects of this method, and the proposed assertion is not related to the primary PR changes.

Sorry, the above recommendation was based on my incorrect assumption that the official code already dereferenced the value pointer. I was wrong. Yes, please refactor as you suggested as the next step.

@ankor2023 ankor2023 Apr 30, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@rousskov

Yes, please refactor as you suggested as the next step.

Done.

}

/// option value to configure one or more characters (e.g., -m=",;")
Expand Down
2 changes: 1 addition & 1 deletion src/acl/Note.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ Acl::NoteCheck::match(ACLChecklist * const ch)
bool
Acl::NoteCheck::matchNotes(const NotePairs *note) const
{
const NotePairs::Entries &entries = note->expandListEntries(&delimiters.value);
const NotePairs::Entries &entries = note->expandListEntries(delimiters.value());
for (auto e: entries)
if (data->match(e.getRaw()))
return true;
Expand Down
24 changes: 13 additions & 11 deletions src/acl/Options.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,14 +101,8 @@ class OptionValue
public:
typedef Value value_type;

// TODO: Some callers use .value without checking whether the option is
// enabled(), accessing the (default-initialized or customized) default
// value that way. This trick will stop working if we add valued options
// that can be disabled (e.g., --with-foo=x --without-foo). To support such
// options, store the default value separately and provide value accessor.

OptionValue(): value {} {}
explicit OptionValue(const Value &aValue): value(aValue) {}
OptionValue(): explicitOrDefaultValue {} {}
explicit OptionValue(const Value &aValue): explicitOrDefaultValue(aValue) {}

/// whether the option is explicitly turned "on" (with or without a value)
bool enabled() const { return configured && !disabled; }
Expand All @@ -117,11 +111,19 @@ class OptionValue
/// go back to the default-initialized state
void reset() { *this = OptionValue<Value>(); }

Value value; ///< final value storage, possibly after conversions
const Value *value() const { return enabled() ? &explicitOrDefaultValue : nullptr; }; ///< accessor

@yadij yadij Apr 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
const Value *value() const { return enabled() ? &explicitOrDefaultValue : nullptr; }; ///< accessor
/// \retval nullptr Do not use any value for this option.
const Value *value() const { return enabled() ? &explicitOrDefaultValue : nullptr; };

@rousskov rousskov Apr 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am against the suggestion in this change request because it contains arguably misleading caller instructions (where no instructions are necessary at all) and because the suggested wording implies that a caller has access to multiple option values. I do agree that "accessor" is also problematic and posted a change request with a specific replacement suggestion.

@yadij yadij Apr 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There is no change from the implications created by the member naming ( "AorB" ) except to clarify that hidden nullptr semantic meaning. I am open to alternatives that clarify the three semantic meanings better.

@ankor2023 ankor2023 Apr 28, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@yadij

There is no change from the implications created by the member naming ( "AorB" ) except to clarify that hidden nullptr semantic meaning. I am open to alternatives that clarify the three semantic meanings better.

Alex suggested the following comment for this method:

/// option value (if the option was enabled) or nil (otherwise)
const Value *value() const { return enabled() ?  &explicitOrDefaultValue : nullptr; }

and for the variable description:

/// When 'valued' is true, this is an explicitly set value.
/// Otherwise, this is the default value.
Value explicitOrDefaultValue;

In my opinion, this covers the three possible semantic meanings quite well.

Comment thread
ankor2023 marked this conversation as resolved.
Outdated
void setValue(const Value &aValue){ explicitOrDefaultValue = aValue; };

bool configured = false; ///< whether the option was present in squid.conf
/* flags for configured options */
bool disabled = false; ///< whether the option was turned off
bool valued = false; ///< whether a configured option had a value

private:
/// Final value storage, possibly after conversions.
Comment thread
ankor2023 marked this conversation as resolved.
Outdated
/// When `valued` is true, this is an explicitly set value.
/// Otherwise, this is the default value.
Value explicitOrDefaultValue;
};

/// a type-specific Option (e.g., a boolean --toggle or -m=SBuf)
Expand Down Expand Up @@ -191,8 +193,8 @@ class TypedOption: public Option
}

private:
void import(const SBuf &rawValue) const { recipient_->value = rawValue; }
void printValue(std::ostream &os) const { os << recipient_->value; }
void import(const SBuf &rawValue) const { recipient_->setValue(rawValue); }
void printValue(std::ostream &os) const { os << *recipient_->value(); }

// The "mutable" specifier demarcates set-once Option kind/behavior from the
// ever-changing recipient of the actual admin-configured option value.
Expand Down
Loading