Skip to content

Refactor Mgr::StringParam.str to be a SBuf [DRAFT] #2405

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

Draft
kinkie wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Refactor Mgr::StringParam.str to be a SBuf [DRAFT] #2405

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
250edcb
Refactor Mgr::StringParam.str to be a SBuf
kinkie Apr 10, 2026
fb7a376
Mgr::StringParam::value() can return a SBuf instead of a ref
kinkie Apr 14, 2026
481ef26
inline SBuf
kinkie Apr 14, 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
24 changes: 24 additions & 0 deletions src/ipc/TypedMsgHdr.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include "squid.h"
#include "base/TextException.h"
#include "ipc/TypedMsgHdr.h"
#include "sbuf/SBuf.h"
#include "SquidString.h"
#include "tools.h"

Expand Down Expand Up @@ -147,6 +148,29 @@ Ipc::TypedMsgHdr::putString(const String &s)
putRaw(s.rawBuf(), s.psize());
}

void
Ipc::TypedMsgHdr::getSBuf(SBuf &s) const
{
const int length = getInt();
Must(length >= 0);
Copy link
Copy Markdown
Contributor

@rousskov rousskov Apr 13, 2026

Choose a reason for hiding this comment

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

Here and elsewhere, please avoid deprecated Must() in new code. Throw a TextException instead.

// SBuf doesn't need special handling for empty
if (!length) {
s.clear();
return;
}
Comment on lines +156 to +160
Copy link
Copy Markdown
Contributor

@rousskov rousskov Apr 13, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

The if statement contradicts its comment/description. Please either remove the comment or remove both the comment and the statement. I do recommend removing both.

Suggested change
// SBuf doesn't need special handling for empty
if (!length) {
s.clear();
return;
}
if (!length) {
s.clear();
return;
}

or

Suggested change
// SBuf doesn't need special handling for empty
if (!length) {
s.clear();
return;
}

or

Suggested change
// SBuf doesn't need special handling for empty
if (!length) {
s.clear();
return;
}
// zero length OK

Must(length <= maxSize);
s.assign(data.raw + offset, length);
offset += length;
}

void
Ipc::TypedMsgHdr::putSBuf(const SBuf &s)
{
Must(s.length() <= maxSize);
putInt(s.length());
putRaw(s.rawContent(), s.length());
}

void
Ipc::TypedMsgHdr::getFixed(void *rawBuf, size_t rawSize) const
{
Expand Down
3 changes: 3 additions & 0 deletions src/ipc/TypedMsgHdr.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@

#include <type_traits>

class SBuf;
class String;

namespace Ipc
Expand Down Expand Up @@ -57,6 +58,8 @@ class TypedMsgHdr: public msghdr
/* access to message parts for selected commonly-used part types */
void getString(String &s) const; ///< load variable-length string
void putString(const String &s); ///< store variable-length string
void getSBuf(SBuf &s) const; ///< load variable-length SBuf
void putSBuf(const SBuf &s); ///< store variable-length SBuf
Comment on lines +61 to +62
Copy link
Copy Markdown
Contributor

@rousskov rousskov Apr 13, 2026

Choose a reason for hiding this comment

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

I recommend overloading these methods. They are not about the exact storage type (which may change). They are about extracting a {length, data} field.

Suggested change
void getSBuf(SBuf &s) const; ///< load variable-length SBuf
void putSBuf(const SBuf &s); ///< store variable-length SBuf
void getString(SBuf &) const; ///< load a variable-length field
void putString(const SBuf &); ///< store a variable-length field

Overloading would also reduce noise when callers are upgraded to use SBuf.

int getInt() const; ///< load an integer
void putInt(int n); ///< store an integer
void getFixed(void *raw, size_t size) const; ///< always load size bytes
Expand Down
2 changes: 1 addition & 1 deletion src/mgr/QueryParams.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ ParseParamValue(const SBuf &rawValue)
if (tok.atEnd())
return new Mgr::IntParam(array);
else
return new Mgr::StringParam(SBufToString(rawValue));
return new Mgr::StringParam(rawValue);
}

/**
Expand Down
13 changes: 3 additions & 10 deletions src/mgr/StringParam.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ Mgr::StringParam::StringParam():
{
}

Mgr::StringParam::StringParam(const String& aString):
Mgr::StringParam::StringParam(const SBuf& aString):
QueryParam(QueryParam::ptString), str(aString)
{
}
Expand All @@ -26,18 +26,11 @@ void
Mgr::StringParam::pack(Ipc::TypedMsgHdr& msg) const
{
msg.putPod(type);
msg.putString(str);
msg.putSBuf(str);
}

void
Mgr::StringParam::unpackValue(const Ipc::TypedMsgHdr& msg)
{
msg.getString(str);
msg.getSBuf(str);
}

const String&
Mgr::StringParam::value() const
{
return str;
}

8 changes: 4 additions & 4 deletions src/mgr/StringParam.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
#include "ipc/forward.h"
#include "mgr/forward.h"
#include "mgr/QueryParam.h"
#include "SquidString.h"
#include "sbuf/SBuf.h"

namespace Mgr
{
Expand All @@ -23,13 +23,13 @@ class StringParam: public QueryParam
{
public:
StringParam();
StringParam(const String& aString);
StringParam(const SBuf& aString);
void pack(Ipc::TypedMsgHdr& msg) const override;
void unpackValue(const Ipc::TypedMsgHdr& msg) override;
const String& value() const;
const auto& value() const { return str; }

private:
String str;
SBuf str;
};

} // namespace Mgr
Expand Down
6 changes: 0 additions & 6 deletions src/tests/stub_libmgr.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,6 @@ void Mgr::MenuAction::dump(StoreEntry *) STUB

Mgr::Action::Pointer Mgr::ShutdownAction::Create(const Mgr::CommandPointer &) STUB_RETVAL(dummyAction)
void Mgr::ShutdownAction::dump(StoreEntry *) STUB
// protected:
//Mgr::ShutdownAction::ShutdownAction(const CommandPointer &) STUB

Mgr::Action::Pointer Mgr::ReconfigureAction::Create(const Mgr::CommandPointer &) STUB_RETVAL(dummyAction)
void Mgr::ReconfigureAction::dump(StoreEntry *) STUB
Expand Down Expand Up @@ -241,10 +239,6 @@ void Mgr::StoreToCommWriter::noteCommClosed(const CommCloseCbParams&) STUB
void Mgr::StoreToCommWriter::close() STUB

#include "mgr/StringParam.h"
//Mgr::StringParam::StringParam() STUB
//Mgr::StringParam::StringParam(const String&) STUB
void Mgr::StringParam::pack(Ipc::TypedMsgHdr&) const STUB
void Mgr::StringParam::unpackValue(const Ipc::TypedMsgHdr&) STUB
static String t;
const String& Mgr::StringParam::value() const STUB_RETVAL(t)

Loading