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 3 commits into
base: master
Choose a base branch
from
+22 −17
Open

Fix default -m delimiter for note ACL #2410

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 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
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]";
Copy link
Copy Markdown
Contributor

@rousskov rousskov Apr 23, 2026

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.

Copy link
Copy Markdown
Contributor Author

@ankor2023 ankor2023 Apr 28, 2026

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.
}

Copy link
Copy Markdown
Contributor

@rousskov rousskov Apr 28, 2026

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.

}

/// 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
26 changes: 15 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,21 @@ class OptionValue
/// go back to the default-initialized state
void reset() { *this = OptionValue<Value>(); }

Value value; ///< final value storage, possibly after conversions
/// option value (if the option was enabled) or nil (otherwise)
const Value *value() const { return enabled() ? &explicitOrDefaultValue : nullptr; }
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:
/// Value storage after any conversions.
/// For an example of a conversion, \see TypedOption<CharacterSetOptionValue>::import().
/// 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 +195,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