Re: StringNg review: MemBlob
On 22/09/10 16:31, Alex Rousskov wrote:
On 09/20/2010 08:18 PM, Alex Rousskov wrote:
On 09/03/2010 09:21 AM, Kinkie wrote:
You'll find the branch at lp:~kinkie/squid/stringng
...
Comments for MemBlob.cci r9472:
Found one more:
* \note memcpy is used as the copying function. This is safe,
* because it is guarranteed by design that the copied-to
* memory area is only owned by the appended-to string,
* and thus doesn't overlap with the appended-from area
*/
void
MemBlob::append(const char *S, const MemBlob::size_type Ssize)
{
memcpy(mem+bufUsed,S,Ssize);
bufUsed+=Ssize;
++stats.append;
}
The \note is correct,
No. As you pointed out to me earlier with SBuf the S here may be the
result of:
a.clear(); a.append(a.mem, 1);
...
but we should not mention string in MemBlob.cci.
MemBlob does not (or should not) know what its users are. There may be a
better way to express the same thought. Consider:
\none memcpy() is safe because we copy to an unused area
*that* is better. There is still no guarantee it wont overflow on the
destination. memcpy() makes no mention about when happens when one of
the pointers is NULL, but experience shows a lot of segfaults.
memcpy() docs state always copies exactly num bytes and that the
buffers should not overlap.
memmove() is the safe one which should be used if there is any doubt
about overlapping memory. But even that can't validate against overflows.
The note belongs to the append() implementation, not its description.
Amos
--
Please be using
Current Stable Squid 2.7.STABLE9 or 3.1.8
Beta testers wanted for 3.2.0.2
Re: StringNg review: MemBlob
On Tue, Sep 21, 2010 at 4:18 AM, Alex Rousskov
rouss...@measurement-factory.com wrote:
On 09/03/2010 09:21 AM, Kinkie wrote:
You'll find the branch at lp:~kinkie/squid/stringng
I decided to take a look at MemBlob because it is used by SBuf which was
reviewed a few days ago. It would be nice to get a fixed SBuf committed, but
that would require committing the classes it uses.
Committing smaller self-contained parts may be an overall better strategy
than trying to fix everything first and then do one huge commit.
So here is a MemBlob review:
MemBlob and SBuf are meant to be used together. They split the burden
of managing memory correcly.
Comments for src/MemBlob.h r9472:
#include MemPool.h
Needed for MEMPROXY_CLASS etc
Remove if not needed.
#include RefCount.h
/**
* A container for various MemBlob class-wide statistics
*/
class MemBlobStats {
public:
u_int64_t alloc; ///number of allocation operations
Class allocations or buffer allocations?
Clarified. MemBlob allocations
u_int64_t realloc; ///number of re-allocation operations
.
u_int64_t live; ///number of live references to bufs
How can you know the number of references? Do you mean the number of MemBob
instances in existence?
Number of MemBlob instances. Clarified.
u_int64_t append; ///number of append operations (whtout grow)
spelling
Besides, MemBob does not grow!
Fixed
u_int64_t allocatedBytes; ///the total size of the allocated storage
Conflicts with the previous use of allocation. If that is not changed,
use liveBytes here.
Done.
/**
* Dumps statistics to an ostream. Use an ostringstream to
* capture them to a string
*/
std::ostream dump(std::ostream os) const;
Consider removing reference to ostringstream or moving it to some .dox file.
It does not belong to interface documentation.
Fixed.
MemBlobStats();
};
.
/**
* Low-level buffer. It's responsible for allocating and managing
* the blocks of memory. It's refcounted, but it doesn't know
* which SBufs hold references to it.
*/
MemBlob does not manage blocks, it manages one block. It does not know many
things (do not document what is not there).
Fixed
Consider:
/// A refcounted fixed-size buffer. Tracks the portion used by opaque
content.
How about:
Refcounted memory buffer, content-agnostic. Meant to only be used by SBuf. ?
class MemBlob: public RefCountable {
public:
typedef RefCountMemBlob Pointer;
typedef unsigned int size_type;
Would be nice to guarantee support for 5GB and larger blobs.
Do we need them now? It'll be trivial to extend when needed.
char *mem; /// The block of memory
Missing in Doxygen comments?
Yes. I wasn't aware of the construct.
Consider: /// raw allocated memory
size_type bufSize; /// Size of the allocated memory
size_type bufUsed; /// How much of the memory is actually used
Other Squid code is using capacity and size for this. Std::string also uses
size() for user-facing buffer length. Please be consistent.
Changed, even though I find the name size confusing in this case.
Remove the buf prefix until you have some other size and capacity members
to distinction from. Besides, this is not a buf. This is a blob :-)
Yup. Relics... I've been working on this for more than two years now.
I would not leave these data members public, but it is your call.
This class originally was private to SBuf. I've now clarified in the
class definition that it's not meant for general use; its only client
should be SBuf.
_SQUID_INLINE_ void deleteSelf();
I hope this can be removed. If not, it must be documented. The corresponding
comments in .cci do not make sense to me and do not match RefCount.h in
trunk, as far as I can tell.
http://old.squid-cache.org/Doc/Prog-Guide/prog-guide-24.html
http://markmail.org/message/kt7357pwttmzrey3
Maybe the documentaiton is outdated?
MemBlob(const size_type size);
Missing explicit. It is better to document whether it allocates anything.
Fixed.
MemBlob(char * memBlock, const size_type memSize);
Not sure why this is needed, but needs to be documented, especially with
regard to memBlock ownership and destruction rules.
Documented.
Please keep in mind that without a default constructor (and reserve()), it
would be impossible to have containers of blobs.
Not needed. We want containers of SBufs, a MemBlob is more akin to a
smart struct.
_SQUID_INLINE_ bool isAtEnd(const char *ptr) const;
Missing documentation.
Why is this needed? Some transitional call? If yes, document as such.
It is needed to know whether a the tail of a SBuf is at the tail of a
shared MemBlob. Such a SBuf can append without needing a cow, if
there's enough headroom in the MemBlob.
_SQUID_INLINE_ size_type freeSpace() const;
Missing documentation.
Renamed to getAvailableSpace. Documentation was in the .cci, moved it
This reads like a command
Re: StringNg review: MemBlob
On Wed, Sep 22, 2010 at 8:47 AM, Amos Jeffries squ...@treenet.co.nz wrote:
On 22/09/10 16:31, Alex Rousskov wrote:
On 09/20/2010 08:18 PM, Alex Rousskov wrote:
On 09/03/2010 09:21 AM, Kinkie wrote:
You'll find the branch at lp:~kinkie/squid/stringng
...
Comments for MemBlob.cci r9472:
Found one more:
* \note memcpy is used as the copying function. This is safe,
* because it is guarranteed by design that the copied-to
* memory area is only owned by the appended-to string,
* and thus doesn't overlap with the appended-from area
*/
void
MemBlob::append(const char *S, const MemBlob::size_type Ssize)
{
memcpy(mem+bufUsed,S,Ssize);
bufUsed+=Ssize;
++stats.append;
}
The \note is correct,
No. As you pointed out to me earlier with SBuf the S here may be the result
of:
a.clear(); a.append(a.mem, 1);
Using SBuf's raw memory access functions is dangerous, and this is
well documented.
It gives users rope, can't be blamed for how they use it..
...
but we should not mention string in MemBlob.cci.
MemBlob does not (or should not) know what its users are. There may be a
better way to express the same thought. Consider:
\none memcpy() is safe because we copy to an unused area
*that* is better. There is still no guarantee it wont overflow on the
destination. memcpy() makes no mention about when happens when one of the
pointers is NULL, but experience shows a lot of segfaults.
Changed to reflect this.
MemBlob.mem can never be NULL; we could add a guard if the user passes
a NULL char*, and treat it as a NOP.
memcpy() docs state always copies exactly num bytes and that the buffers
should not overlap.
memmove() is the safe one which should be used if there is any doubt about
overlapping memory. But even that can't validate against overflows.
Can change to memmove; is there a performance hit?
--
/kinkie
Re: [PATCH] comm cleanup mk8 - src/comm only
On 21/09/10 10:00, Alex Rousskov wrote:
On 09/19/2010 05:35 AM, Amos Jeffries wrote:
Round 2 of only src/comm/ changes.
I believe this accounts for all the bits requested to date. Including
adding Subscriptions for ConnAcceptor.
* The patch is missing the new Subscription class. Based on your subject
line, I am guessing you will post that separately.
* Comm::ConnOpener::start() should call connect() directly. Start() is
already async, there is no need to pay the async price again. Moreover,
by doing an async call, you are more likely to encounter timeouts even
before we call connect() because the clock starts ticking before the
call is fired.
Done.
* Comm::ConnOpener::timeout() should call connect() directly. Timeout()
is already async, there is no need to pay the async price again.
Done.
* I do not like how Comm::ConnOpener::connect() calls itself. I assume
you did not invent this, but the code will fail to produce the
intended(?) delay if there are no other async calls already scheduled,
which is something the class does not really control. Recall that async
calls are AsyncCallQueue::fire()d until there are none left...
At the very least, please add a comment explaining why we use such a
hack instead of either using an explicit loop or waiting a little bit
via eventAdd.
I think you asked me to remove eventAdd a while back before this was a
Job. Reverted to the old .05 sec delay.
* calls_.connect_ should not be reused in Comm::ConnOpener::connect()
because AsyncCall objects may not support being fired twice -- they may
have state. The connect_ object is not needed at all if you address the
above comments. If you do not, set it to NULL at the start of
Comm::ConnOpener::connect() and recreate when needed.
Oops. sorry. Foxed.
* I may be wrong, but the interesting Is opened conn fully open?
comment below may reveal a couple of bugs:
+ // recover what we can from the job
+ if (conn_ != NULL conn_-isOpen()) {
+ // it never reached fully open, so abort the FD
+ conn_-close();
+ ...
Clearly, conn_ is open inside that if-statement (there is no such thing
as half-open conn, I hope; we have enough headaches from half-closed
sorry, no such luck. see COMM_INPROGRESS.
As I understand it:
For the period between comm_connect_addr() returning and a signal
being received (via the write handler being called) that write access is
possible the FD is in fact half-open.
The kernel seems to have allocated a FD and returned (non-blocking)
but still be awaiting the network SYN-ACK.
The headaches here are small since only the opening sequence job and the
fd_table is involved. We just have to cleanup the conn_-fd before
passing control of conn_ back out to any other part of Squid.
ones!). There is nothing wrong with the connection. Instead, it is the
ConnOpener job itself that got into a half-finished state here, most
Yes. Nothing wrong, but we can't use the FD the system gave us already.
likely due to an exception after the connection got opened and before it
was sent to the caller. To clean up, two separate actions are needed:
a) Close the opened connection. The beginning of your if-statement will
do the job, but do not call doneConnecting(). Here is a polished version:
if (conn_ != NULL conn_-isOpen()) {
Must(callback_ != NULL); // or there should be no conn_
// we never sent this open connection to the caller, so close
conn_-close();
fd_table[conn_-fd].flags.open = 0;
conn_ = NULL;
}
b) Notify the caller we are dying:
if (callback_ != NULL) {
// do not let the caller wait forever
doneConnecting(COMM_ERR_CONNECT, 0);
}
I am not sure about the order. The above order means caller can handle
and should get nil params.conn. You can remove conn_ = NULL if
caller should get a non-nil but closed params.conn, but there will
still be two if-statements.
Good idea. Above order is correct. params.conn may contain the attempted
local/remote IP combos for use by the callers recovery. !COMM_OK status
guarantees that the FD is not safely usable.
I also see that I missed clearing the write handler before fully closing
conn_. do the timeout and closing handlers need to be explicitly
removed? or are they happy with the cancels?
* Consider moving case COMM_OK code, including lookupLocalAddress() to
something like Connection::opened(). The code seems to be manipulating
conn_ without access to Comm::ConnOpener data except perhaps host_. I
could have missed some other dependencies, of course, but it feels like
a Connection business to set things up after the connection has been
established. The ConnOpener job is done at that stage.
Done.
* Can we leave resetting fd_table[conn_-fd].flags.open to comm_close(),
which will be called from via conn_-close()? It feels wrong to force it
to zero way before the connection is actually closed but I understand
that you may be fighting some hidden dependencies.
We can. It was me being paranoid rather than known problems.
Considering
Re: StringNg review: MemBlob
On 22/09/10 21:30, Kinkie wrote:
On Tue, Sep 21, 2010 at 4:18 AM, Alex Rousskov
rouss...@measurement-factory.com wrote:
On 09/03/2010 09:21 AM, Kinkie wrote:
You'll find the branch at lp:~kinkie/squid/stringng
I decided to take a look at MemBlob because it is used by SBuf which was
reviewed a few days ago. It would be nice to get a fixed SBuf committed, but
that would require committing the classes it uses.
Committing smaller self-contained parts may be an overall better strategy
than trying to fix everything first and then do one huge commit.
So here is a MemBlob review:
snip
Remove the buf prefix until you have some other size and capacity members
to distinction from. Besides, this is not a buf. This is a blob :-)
Yup. Relics... I've been working on this for more than two years now.
I would not leave these data members public, but it is your call.
This class originally was private to SBuf. I've now clarified in the
class definition that it's not meant for general use; its only client
should be SBuf.
If still relevant that would be reason for protected: and friend
declarations in the class definition instead of public:.
_SQUID_INLINE_ void deleteSelf();
I hope this can be removed. If not, it must be documented. The corresponding
comments in .cci do not make sense to me and do not match RefCount.h in
trunk, as far as I can tell.
http://old.squid-cache.org/Doc/Prog-Guide/prog-guide-24.html
http://markmail.org/message/kt7357pwttmzrey3
Maybe the documentaiton is outdated?
Seems to be. The RefCountable classes no longer have deleteSelf.
_SQUID_INLINE_ bool isAtEnd(const char *ptr) const;
Missing documentation.
Why is this needed? Some transitional call? If yes, document as such.
It is needed to know whether a the tail of a SBuf is at the tail of a
shared MemBlob. Such a SBuf can append without needing a cow, if
there's enough headroom in the MemBlob.
So, a char* way of saying:
if (SBuf::off_+Sbuf::len_ == MemBlob::size
SBuf::off_+Sbuf::len_ MemBlob::capacity) {
...
with canAppend being a duplicate as above but = MemBlob::capacity+N
or rather isAtEnd being a case of canAppend(0)
This reads like a command to free some space.
Consider: spaceSize()
spaceSize() would be more consistent with other Squid code as well.
_SQUID_INLINE_ bool canGrow(const size_type howmuch) const;
Missing documentation.
Usually unsafe and wastefull. See similar comments for SBuf.
It'd be unsafe in a concurrent (multithreaded) environment; how would
it be unsafe in a single-threaded env?
Squid won't be single-threaded for much longer. At least on the
timescale for string-level changes.
_SQUID_INLINE_ bool canAppend(char const * bufEnd, const size_type
howmuch) const;
Missing documentation.
Especially since it is not clear why canAppend() needs a pointer.
Looking at the .cci implementation, I question why would the caller
keep the bufEnd pointer if MemBlob manages it already? I do not think this
call or duplicated bufEnd pointer is needed.
if many SBufs reference the same MemBlob, an append operation to one
of them does not need a cow if the appended-to SBuf is at the tail of
the MemBlob and there is enough free space at the end of the MemBlob
to hold the data being appended.
This canAppend call checks exactly that. the SBuf knows its own tail,
while the MemBlob knows the tail of the used portion of itself.
see isAtEnd() comment above.
/**
* constructor, creates an empty MemBlob of size= requested
* \param size the minimum size to be guarranteed from the MemBlob
*/
MemBlob::MemBlob(const MemBlob::size_type size)
{
debugs(MEMBLOB_DEBUGSECTION,9,HERE size= size);
memAlloc(size);
}
I would recommend initializing mem and sizes to NULL/0s explicitly in all
the constructors. It does not cost much and may save a few hours of
debugging if things go wrong.
Isn't setting them in memAlloc enough? Besides the actual size may be
different (or would you mean to set them twice?)
In the constructor init list.
You can then enforce the MUST and ONLY requirements with
assert(mem==NULL) etc at the start of memAlloc.
Amos
--
Please be using
Current Stable Squid 2.7.STABLE9 or 3.1.8
Beta testers wanted for 3.2.0.2
Re: StringNg review: MemBlob
On 09/22/2010 03:30 AM, Kinkie wrote:
On Tue, Sep 21, 2010 at 4:18 AM, Alex Rousskov wrote:
On 09/03/2010 09:21 AM, Kinkie wrote:
You'll find the branch at lp:~kinkie/squid/stringng
/**
* Low-level buffer. It's responsible for allocating and managing
* the blocks of memory. It's refcounted, but it doesn't know
* which SBufs hold references to it.
*/
Consider:
/// A refcounted fixed-size buffer. Tracks the portion used by opaque
content.
How about:
Refcounted memory buffer, content-agnostic. Meant to only be used by SBuf. ?
Fixed-size is an key property unless you think it may change soon.
Content-agnostic is fine, I guess.
As for meant to only be used by Foo, I am still against such
restrictions on use in general and in this context in particular. We
have discussed this before. Use-cases may determine the initial design
sketch and optimizations, but the final APIs should be usable in any
context that needs them rather than restricted to one specific user.
MemBlob is no exception IMO.
class MemBlob: public RefCountable {
public:
typedef RefCountMemBlob Pointer;
typedef unsigned int size_type;
Would be nice to guarantee support for 5GB and larger blobs.
Do we need them now? It'll be trivial to extend when needed.
I may need them soon. Based on past experience, I am not convinced that
replacing 'int' with some other type will be trivial.
I would not leave these data members public, but it is your call.
This class originally was private to SBuf. I've now clarified in the
class definition that it's not meant for general use; its only client
should be SBuf.
Which is a wrong restriction made worse by wrongly guessed implications.
As SBuf review shows, we are already paying the price for the lack of
proper boundaries between the two classes. It makes some areas of the
SBuf code confusing at best.
_SQUID_INLINE_ void deleteSelf();
I hope this can be removed. If not, it must be documented. The corresponding
comments in .cci do not make sense to me and do not match RefCount.h in
trunk, as far as I can tell.
http://old.squid-cache.org/Doc/Prog-Guide/prog-guide-24.html
http://markmail.org/message/kt7357pwttmzrey3
Maybe the documentaiton is outdated?
That old documentation is wrong. Use the source code. Remove deleteSelf().
_SQUID_INLINE_ size_type freeSpace() const;
Missing documentation.
Renamed to getAvailableSpace. Documentation was in the .cci, moved it
Wrong name. The method does not give the caller the available space,
only its size. Here are some combinations that work better:
size(), spaceSize(), capacity()
contentSize(), spaceSize(), capacity()
usedSize() and availableSize(), capacity()
MemBlobStats MemBlob::stats;
If some static code wants to allocate MemBlob, will its stats not get
counted, depending on static initialization order?
Yes. I don't think that's much of an issue;
It becomes an issue when we get bug reports about negative or too-big
counters. Or when somebody adds an assert() that the counter is correct.
Both have happened before.
stats are mostly meant to
help us diagnose the effectiveness of the solution, and in fact I
wouldn't be against #ifdef-ing them out in production code.
Sure, nobody cares about effectiveness of the code in production as long
as it was effective in the lab :-).
/**
* constructor, creates an empty MemBlob of size= requested
* \param size the minimum size to be guarranteed from the MemBlob
*/
MemBlob::MemBlob(const MemBlob::size_type size)
{
debugs(MEMBLOB_DEBUGSECTION,9,HERE size= size);
memAlloc(size);
}
I would recommend initializing mem and sizes to NULL/0s explicitly in all
the constructors. It does not cost much and may save a few hours of
debugging if things go wrong.
Isn't setting them in memAlloc enough? Besides the actual size may be
different (or would you mean to set them twice?)
Initialize them to NULLs and 0s before calling memAlloc. Such
initializations cost little and avoid having an object with
uninitialized members.
Thank you,
Alex.
Re: [PATCH] comm cleanup mk8 - src/comm only
On 09/22/2010 08:28 AM, Amos Jeffries wrote:
On 21/09/10 10:00, Alex Rousskov wrote:
On 09/19/2010 05:35 AM, Amos Jeffries wrote:
* I do not like how Comm::ConnOpener::connect() calls itself. I assume
you did not invent this, but the code will fail to produce the
intended(?) delay if there are no other async calls already scheduled,
which is something the class does not really control. Recall that async
calls are AsyncCallQueue::fire()d until there are none left...
At the very least, please add a comment explaining why we use such a
hack instead of either using an explicit loop or waiting a little bit
via eventAdd.
I think you asked me to remove eventAdd a while back before this was a
Job. Reverted to the old .05 sec delay.
I could have made the wrong call, you could have interpreted my comment
incorrectly, or both. If you want to review what happened, please find
and repost that conversation.
* I may be wrong, but the interesting Is opened conn fully open?
comment below may reveal a couple of bugs:
+ // recover what we can from the job
+ if (conn_ != NULL conn_-isOpen()) {
+ // it never reached fully open, so abort the FD
+ conn_-close();
+ ...
Clearly, conn_ is open inside that if-statement (there is no such thing
as half-open conn, I hope; we have enough headaches from half-closed
sorry, no such luck. see COMM_INPROGRESS.
I see. Fortunately, my swanSong() fix still applied.
I also see that I missed clearing the write handler before fully closing
conn_. do the timeout and closing handlers need to be explicitly
removed? or are they happy with the cancels?
AFAIK, comm_close() will remove those handlers automagically because it
will call them with COMM_ERR_CLOSING. However, our code needs to make
sure that it can handle such callbacks until swanSong() is called.
Considering no other jobs etc have anything validly pending on this
brand new FD should I be calling fd_close(conn_-fd) here instead of
conn_-close()?
In general, if you used comm_open, use comm_close.
If comm_close works, let's use it, avoiding low-level calls we should
not even have access to. Comm itself may have something pending on that
new FD...
* Can you explain how syncWithComm() side effects hit
theCallSub-callback()? Is not theCallSub-callback() itself constant
and its result is stored in a local variable? I do not understand how
syncWithComm() can affect theCallSub itself. Thank you.
Possibly obsolete now.
Dialer copy-constructors was initially created using getDialer() which
call syncWithComm() which in CommIoCb child class updates this-* state.
Fallout of that is that:
the CommIoCb constructor cant take a const parameter,
the parent class cant provide a const overload constructor,
the other children cant either,
callback() using copy-constructor cant be const.
I *think* the copy constructors are okay using raw dialer pointers now.
Which means everything down the chain can be const.
I cannot comment on that without seeing the new call-copying code.
* cbdataReferenceDone() already sets its parameter to NULL. No need to
do that twice:
+ /* clear any previous ptr */
+ if (_peer) {
+ cbdataReferenceDone(_peer);
+ _peer = NULL;
+ }
Great.
At this random point we are not sure its valid. For now I've make the
if() use the fixed getPeer().
That would leak _peer if it became invalid (unless you call
cbdataReferenceDone(_peer) in getPeer() now, which would be technically
correct and efficient but weird).
Would it be correct to use
cbdataReferenceValidDone(NULL, _peer) unconditionally here with no
callback? instead of separate validate and done?
You do not care about _peer validity or value because you are not
dereferencing that pointer. Just do:
cbdataReferenceDone(_peer);
No ifs, no setting to NULL, no getPeer().
* Consider s/_peer/peer_/ in Connection for consistency with most other
code.
The struct peer_ global symbol clashes and gcc complains about missing
; or = after variable struct.
I don't plan on fighting that one just yet. Will mark it to be done though.
Agreed. The struct should be renamed (to Peer?), but that is not your
problem. You can use just peer for the member name, I guess.
* s/unused//g. C++ does not require naming unused parameters.
Done. And converted the two places (commSetSelect and eventAdd wrappers)
to use JobCallback with NullaryMemFunT dialers on connect().
Not sure what you mean. Will become clear when the code is posted, I guess.
Cheers,
Alex.
Re: StringNg review: MemBlob
On 09/22/2010 12:47 AM, Amos Jeffries wrote:
On 22/09/10 16:31, Alex Rousskov wrote:
On 09/20/2010 08:18 PM, Alex Rousskov wrote:
On 09/03/2010 09:21 AM, Kinkie wrote:
You'll find the branch at lp:~kinkie/squid/stringng
...
Comments for MemBlob.cci r9472:
Found one more:
* \note memcpy is used as the copying function. This is safe,
* because it is guarranteed by design that the copied-to
* memory area is only owned by the appended-to string,
* and thus doesn't overlap with the appended-from area
*/
void
MemBlob::append(const char *S, const MemBlob::size_type Ssize)
{
memcpy(mem+bufUsed,S,Ssize);
bufUsed+=Ssize;
++stats.append;
}
The \note is correct,
No. As you pointed out to me earlier with SBuf the S here may be the
result of:
a.clear(); a.append(a.mem, 1);
The situation here is different, I believe, but I do not understand your
example. What is a? MemBlob does not have clear(). SBuf does not have
mem...
If s* are SBufs and you meant
s1.clear(); s1.append(s1.buf(), s1.size())
then it is fine because s1.size() will be zero.
If you meant
s1.clear(); s1.append(s2.buf(), s2.size())
then it is fine because s1.append will COW if s1 and s2 share MemBlob
and s2.size() is positive.
If you mean
blob.clear; blob.append(blob.mem, blob.size())
then it is fine because blob.size() will be zero.
...
but we should not mention string in MemBlob.cci.
MemBlob does not (or should not) know what its users are. There may be a
better way to express the same thought. Consider:
\none memcpy() is safe because we copy to an unused area
*that* is better. There is still no guarantee it wont overflow on the
destination. memcpy() makes no mention about when happens when one of
the pointers is NULL, but experience shows a lot of segfaults.
The MemBlob append() code should not call memcpy() with a NULL
destination pointer, of course, but that is not related to the \note. My
understanding is that blob.mem is never NULL.
The MemBlob append() code must check for overflows. If I did not comment
on that before, please consider this a please fix comment.
Kinkie, while you are at it, please also avoid calling memcpy/move()
when size-to-append is zero, just in case.
memcpy() docs state always copies exactly num bytes and that the
buffers should not overlap.
memmove() is the safe one which should be used if there is any doubt
about overlapping memory. But even that can't validate against overflows.
Per \note, I think we are fine with memcpy() here but counter-examples
are more than welcome. I assume memcpy() is significantly faster in some
environments we care about.
On the other hand, if memmove() implementations we care about always
quickly check for overlaps and actually call memcpy() when possible,
then we should just use memmove()!
Thank you,
Alex.
Re: StringNg review: MemBlob
On 09/22/2010 03:44 AM, Kinkie wrote:
On Wed, Sep 22, 2010 at 8:47 AM, Amos Jeffriessqu...@treenet.co.nz wrote:
On 22/09/10 16:31, Alex Rousskov wrote:
On 09/20/2010 08:18 PM, Alex Rousskov wrote:
On 09/03/2010 09:21 AM, Kinkie wrote:
You'll find the branch at lp:~kinkie/squid/stringng
...
Comments for MemBlob.cci r9472:
Found one more:
* \note memcpy is used as the copying function. This is safe,
* because it is guarranteed by design that the copied-to
* memory area is only owned by the appended-to string,
* and thus doesn't overlap with the appended-from area
*/
void
MemBlob::append(const char *S, const MemBlob::size_type Ssize)
{
memcpy(mem+bufUsed,S,Ssize);
bufUsed+=Ssize;
++stats.append;
}
The \note is correct,
No. As you pointed out to me earlier with SBuf the S here may be the result
of:
a.clear(); a.append(a.mem, 1);
Using SBuf's raw memory access functions is dangerous, and this is
well documented.
It gives users rope, can't be blamed for how they use it..
This has little to do with raw memory access, actually.
If a is SBuf and a.mem is a.buf(), then
a.clear(); a.append(a.buf(), 1);
is wrong not because some areas overlap (they actually do not, see my
earlier response) but because a.size() is less than 1 after a.clear()
and so the caller should not be using 1 as the size-to-append
parameter. In other words, there is a bug, but the bug is not in the
append(), but in the caller code.
I just realized that SBuf uses length() instead of standard size(). I
hope you got the idea though. :-(
Alex.
Does no-store in request imply no-cache?
Hello, One interpretation of RFC 2616 allows the proxy to serve hits when the request contains Cache-Control: no-store. Do you think such an interpretation is valid? no-store The purpose of the no-store directive is to prevent the inadvertent release or retention of sensitive information (for example, on backup tapes). The no-store directive applies to the entire message, and MAY be sent either in a response or in a request. If sent in a request, a cache MUST NOT store any part of either this request or any response to it. Thank you, Alex.
Re: Does no-store in request imply no-cache?
On Wed, Sep 22, 2010 at 8:27 PM, Alex Rousskov rouss...@measurement-factory.com wrote: Hello, One interpretation of RFC 2616 allows the proxy to serve hits when the request contains Cache-Control: no-store. Do you think such an interpretation is valid? no-store The purpose of the no-store directive is to prevent the inadvertent release or retention of sensitive information (for example, on backup tapes). The no-store directive applies to the entire message, and MAY be sent either in a response or in a request. If sent in a request, a cache MUST NOT store any part of either this request or any response to it. Hi, No; IMVHO it means that it can be stored in RAM, but not swapped out to a cache_dir. no-store = no-cache is a conservative (but valid) approximation. -- /kinkie
Re: Does no-store in request imply no-cache?
On 09/22/2010 02:46 PM, Kinkie wrote: On Wed, Sep 22, 2010 at 8:27 PM, Alex Rousskov rouss...@measurement-factory.com wrote: Hello, One interpretation of RFC 2616 allows the proxy to serve hits when the request contains Cache-Control: no-store. Do you think such an interpretation is valid? no-store The purpose of the no-store directive is to prevent the inadvertent release or retention of sensitive information (for example, on backup tapes). The no-store directive applies to the entire message, and MAY be sent either in a response or in a request. If sent in a request, a cache MUST NOT store any part of either this request or any response to it. Hi, No; IMVHO it means that it can be stored in RAM, but not swapped out to a cache_dir. Looks like my question was not clear. Let me try to rephrase: Assume Squid received a regular request and cached (does not matter where) the corresponding response. That request and that response had no Cache-Control headers. Everything is fine and ordinary. Now comes a second request for that cached object. The request has a Cache-Control: no-store header. Can Squid satisfy that no-store request from the cache? Thank you, Alex.
Re: Does no-store in request imply no-cache?
Strictly, as a request directive it means you can't store the response to this request -- it says nothing about whether or not you can satisfy the request from a cache. However, I imagine most people would interpret it as implying no-cache; you're still conformant if you do. See also: http://tools.ietf.org/html/draft-ietf-httpbis-p6-cache-11#section-3.2.1 On 23/09/2010, at 4:27 AM, Alex Rousskov wrote: Hello, One interpretation of RFC 2616 allows the proxy to serve hits when the request contains Cache-Control: no-store. Do you think such an interpretation is valid? no-store The purpose of the no-store directive is to prevent the inadvertent release or retention of sensitive information (for example, on backup tapes). The no-store directive applies to the entire message, and MAY be sent either in a response or in a request. If sent in a request, a cache MUST NOT store any part of either this request or any response to it. Thank you, Alex. -- Mark Nottingham m...@yahoo-inc.com
Re: Does no-store in request imply no-cache?
On 09/22/2010 05:05 PM, Mark Nottingham wrote: Strictly, as a request directive it means you can't store the response to this request -- it says nothing about whether or not you can satisfy the request from a cache. Hi Mark, Let's assume the above is correct and Squid satisfied the no-store request from the cache. Should Squid purge the cached response afterwards? If Squid does not purge, the next regular request will get the same cached response as the no-store request got, kind of violating the MUST NOT store any response to it no-store requirement. If Squid purges, it is kind of silly because earlier requests could have gotten the same sensitive information before the no-store request came and declared the already cached information sensitive. Thank you, Alex. See also: http://tools.ietf.org/html/draft-ietf-httpbis-p6-cache-11#section-3.2.1 On 23/09/2010, at 4:27 AM, Alex Rousskov wrote: Hello, One interpretation of RFC 2616 allows the proxy to serve hits when the request contains Cache-Control: no-store. Do you think such an interpretation is valid? no-store The purpose of the no-store directive is to prevent the inadvertent release or retention of sensitive information (for example, on backup tapes). The no-store directive applies to the entire message, and MAY be sent either in a response or in a request. If sent in a request, a cache MUST NOT store any part of either this request or any response to it. Thank you, Alex. -- Mark Nottingham m...@yahoo-inc.com
Re: Does no-store in request imply no-cache?
On 23/09/2010, at 9:47 AM, Alex Rousskov wrote: Hi Mark, Let's assume the above is correct and Squid satisfied the no-store request from the cache. Should Squid purge the cached response afterwards? If Squid does not purge, the next regular request will get the same cached response as the no-store request got, kind of violating the MUST NOT store any response to it no-store requirement. Sort of, but not really. I agree this could be worded better; we'll work on it. If Squid purges, it is kind of silly because earlier requests could have gotten the same sensitive information before the no-store request came and declared the already cached information sensitive. Agreed. This has been discussed in the WG before (can't remember the ref); basically, it boiled down to each request being independent; you don't want requests affecting other ones (beyond anything, it's a security issue if you allow clients to purge your cache indescriminantly). -- Mark Nottingham m...@yahoo-inc.com
Re: Does no-store in request imply no-cache?
On Wed, 22 Sep 2010 17:47:57 -0600, Alex Rousskov rouss...@measurement-factory.com wrote: On 09/22/2010 05:05 PM, Mark Nottingham wrote: Strictly, as a request directive it means you can't store the response to this request -- it says nothing about whether or not you can satisfy the request from a cache. Hi Mark, Let's assume the above is correct and Squid satisfied the no-store request from the cache. Should Squid purge the cached response afterwards? If Squid does not purge, the next regular request will get the same cached response as the no-store request got, kind of violating the MUST NOT store any response to it no-store requirement. If Squid purges, it is kind of silly because earlier requests could have gotten the same sensitive information before the no-store request came and declared the already cached information sensitive. Thank you, Alex. I take it as meaning only that any new copy received must not be saved or used to update/replace existing copies. Assuming no-cache is updating the existing copy (removal). client A, B, C in sequence with B using no-store fetch the same thing. client C would be getting same response as client A, no problem. client B could have used no-cache+no-store if it wanted new content not shared with C. Amos See also: http://tools.ietf.org/html/draft-ietf-httpbis-p6-cache-11#section-3.2.1 On 23/09/2010, at 4:27 AM, Alex Rousskov wrote: Hello, One interpretation of RFC 2616 allows the proxy to serve hits when the request contains Cache-Control: no-store. Do you think such an interpretation is valid? no-store The purpose of the no-store directive is to prevent the inadvertent release or retention of sensitive information (for example, on backup tapes). The no-store directive applies to the entire message, and MAY be sent either in a response or in a request. If sent in a request, a cache MUST NOT store any part of either this request or any response to it. Thank you, Alex. -- Mark Nottingham m...@yahoo-inc.com
Build failed in Hudson: 3.HEAD-i386-OpenBSD #580
See http://build.squid-cache.org/job/3.HEAD-i386-OpenBSD/580/changes Changes: [Amos Jeffries squ...@treenet.co.nz] Author: Various Translators Translations Update auto-save [Alex Rousskov rouss...@measurement-factory.com] HTTP Compliance: Make date parser stricter to better handle malformed Expires. Check that there is no zone or zone is GMT in parse_date_elements(). Make sure there is no junk at the end of date in parse_date(). This will affect Date, IMS, and other date-carrying header fields recognized by Squid but should not cause any messages to be rejected. Squid would just ignore the malformed headers as if they are not there. Co-Advisor test case: test_case/rfc2616/invalidExpiresMakesStale-rfc1123x [Alex Rousskov rouss...@measurement-factory.com] HTTP Compliance: do not remove ETag header from partial responses RFC 2616 section 10.2.7 says that partial responses MUST include ETag header if it would have been sent in a 200 response to the same request. Co-Advisor test cases: test_case/rfc2616/scHdrs-206-include-explicit-withVary-noIfs test_case/rfc2616/scHdrs-206-include-explicit-woutVary-noIfs test_case/rfc2616/scHdrs-206-include-other-If-Range-strong [Automatic source maintenance squid...@squid-cache.org] SourceFormat Enforcement -- [...truncated 3098 lines...] /usr/local/lib/gcc/i386-unknown-openbsd4.7/4.2.4/../../../libestdc++.so.11.0: warning: strcat() is almost always misused, please use strlcat() /usr/local/lib/gcc/i386-unknown-openbsd4.7/4.2.4/../../../libestdc++.so.11.0: warning: sprintf() is often misused, please use snprintf() Making all in POP3 sed -e 's,[...@]perl[@],/usr/bin/perl,g' ../../../../helpers/basic_auth/POP3/basic_pop3_auth.pl.in basic_pop3_auth || (/bin/rm -f -f basic_pop3_auth ; exit 1) Making all in RADIUS if ccache g++ -DHAVE_CONFIG_H -I../../../.. -I../../../../include -I../../../../src -I../../../include -I/usr/local/include -I../../../../helpers/basic_auth/RADIUS-Wall -Wpointer-arith -Wwrite-strings -Wcomments -Werror -pipe -D_REENTRANT -g -O2 -MT basic_radius_auth.o -MD -MP -MF .deps/basic_radius_auth.Tpo -c -o basic_radius_auth.o ../../../../helpers/basic_auth/RADIUS/basic_radius_auth.cc; then mv -f .deps/basic_radius_auth.Tpo .deps/basic_radius_auth.Po; else rm -f .deps/basic_radius_auth.Tpo; exit 1; fi if ccache g++ -DHAVE_CONFIG_H -I../../../.. -I../../../../include -I../../../../src -I../../../include -I/usr/local/include -I../../../../helpers/basic_auth/RADIUS-Wall -Wpointer-arith -Wwrite-strings -Wcomments -Werror -pipe -D_REENTRANT -g -O2 -MT radius-util.o -MD -MP -MF .deps/radius-util.Tpo -c -o radius-util.o ../../../../helpers/basic_auth/RADIUS/radius-util.cc; then mv -f .deps/radius-util.Tpo .deps/radius-util.Po; else rm -f .deps/radius-util.Tpo; exit 1; fi /bin/sh ../../../libtool --tag=CXX --mode=link ccache g++ -Wall -Wpointer-arith -Wwrite-strings -Wcomments -Werror -pipe -D_REENTRANT -g -O2 -g -o basic_radius_auth basic_radius_auth.o radius-util.o -L../../../lib -lmiscutil ../../../compat/libcompat.la-lm libtool: link: ccache g++ -Wall -Wpointer-arith -Wwrite-strings -Wcomments -Werror -pipe -D_REENTRANT -g -O2 -g -o basic_radius_auth basic_radius_auth.o radius-util.o -Lhttp://build.squid-cache.org/job/3.HEAD-i386-OpenBSD/ws/btlayer-00-default/squid-3.HEAD-BZR/_build/lib -lmiscutil ../../../compat/.libs/libcompat.a -lm /usr/local/lib/gcc/i386-unknown-openbsd4.7/4.2.4/../../../libestdc++.so.11.0: warning: vsprintf() is often misused, please use vsnprintf() basic_radius_auth.o(.text+0x128): In function `main': ../../../../helpers/basic_auth/RADIUS/basic_radius_auth.cc:471: warning: strcpy() is almost always misused, please use strlcpy() /usr/local/lib/gcc/i386-unknown-openbsd4.7/4.2.4/../../../libestdc++.so.11.0: warning: strcat() is almost always misused, please use strlcat() /usr/local/lib/gcc/i386-unknown-openbsd4.7/4.2.4/../../../libestdc++.so.11.0: warning: sprintf() is often misused, please use snprintf() Making all in fake if ccache g++ -DHAVE_CONFIG_H -I../../../.. -I../../../../include -I../../../../src -I../../../include -I/usr/local/include -Wall -Wpointer-arith -Wwrite-strings -Wcomments -Werror -pipe -D_REENTRANT -g -O2 -MT fake.o -MD -MP -MF .deps/fake.Tpo -c -o fake.o ../../../../helpers/basic_auth/fake/fake.cc; then mv -f .deps/fake.Tpo .deps/fake.Po; else rm -f .deps/fake.Tpo; exit 1; fi /bin/sh ../../../libtool --tag=CXX --mode=link ccache g++ -Wall -Wpointer-arith -Wwrite-strings -Wcomments -Werror -pipe -D_REENTRANT -g -O2 -g -o basic_fake_auth fake.o -L../../../lib -lmiscutil ../../../compat/libcompat.la libtool: link: ccache g++ -Wall -Wpointer-arith -Wwrite-strings -Wcomments -Werror -pipe -D_REENTRANT -g -O2 -g -o basic_fake_auth fake.o -Lhttp://build.squid-cache.org/job/3.HEAD-i386-OpenBSD/ws/btlayer-00-default/squid-3.HEAD-BZR/_build/lib -lmiscutil ../../../compat/.libs/libcompat.a
Build failed in Hudson: 3.HEAD-i386-OpenBSD #581
See http://build.squid-cache.org/job/3.HEAD-i386-OpenBSD/581/changes Changes: [Amos Jeffries squ...@treenet.co.nz] Author: Various Translators Translations Update auto-save -- [...truncated 3094 lines...] /usr/local/lib/gcc/i386-unknown-openbsd4.7/4.2.4/../../../libestdc++.so.11.0: warning: strcat() is almost always misused, please use strlcat() /usr/local/lib/gcc/i386-unknown-openbsd4.7/4.2.4/../../../libestdc++.so.11.0: warning: sprintf() is often misused, please use snprintf() Making all in POP3 sed -e 's,[...@]perl[@],/usr/bin/perl,g' ../../../../helpers/basic_auth/POP3/basic_pop3_auth.pl.in basic_pop3_auth || (/bin/rm -f -f basic_pop3_auth ; exit 1) Making all in RADIUS if ccache g++ -DHAVE_CONFIG_H -I../../../.. -I../../../../include -I../../../../src -I../../../include -I/usr/local/include -I../../../../helpers/basic_auth/RADIUS-Wall -Wpointer-arith -Wwrite-strings -Wcomments -Werror -pipe -D_REENTRANT -g -O2 -MT basic_radius_auth.o -MD -MP -MF .deps/basic_radius_auth.Tpo -c -o basic_radius_auth.o ../../../../helpers/basic_auth/RADIUS/basic_radius_auth.cc; then mv -f .deps/basic_radius_auth.Tpo .deps/basic_radius_auth.Po; else rm -f .deps/basic_radius_auth.Tpo; exit 1; fi if ccache g++ -DHAVE_CONFIG_H -I../../../.. -I../../../../include -I../../../../src -I../../../include -I/usr/local/include -I../../../../helpers/basic_auth/RADIUS-Wall -Wpointer-arith -Wwrite-strings -Wcomments -Werror -pipe -D_REENTRANT -g -O2 -MT radius-util.o -MD -MP -MF .deps/radius-util.Tpo -c -o radius-util.o ../../../../helpers/basic_auth/RADIUS/radius-util.cc; then mv -f .deps/radius-util.Tpo .deps/radius-util.Po; else rm -f .deps/radius-util.Tpo; exit 1; fi /bin/sh ../../../libtool --tag=CXX --mode=link ccache g++ -Wall -Wpointer-arith -Wwrite-strings -Wcomments -Werror -pipe -D_REENTRANT -g -O2 -g -o basic_radius_auth basic_radius_auth.o radius-util.o -L../../../lib -lmiscutil ../../../compat/libcompat.la-lm libtool: link: ccache g++ -Wall -Wpointer-arith -Wwrite-strings -Wcomments -Werror -pipe -D_REENTRANT -g -O2 -g -o basic_radius_auth basic_radius_auth.o radius-util.o -Lhttp://build.squid-cache.org/job/3.HEAD-i386-OpenBSD/ws/btlayer-00-default/squid-3.HEAD-BZR/_build/lib -lmiscutil ../../../compat/.libs/libcompat.a -lm /usr/local/lib/gcc/i386-unknown-openbsd4.7/4.2.4/../../../libestdc++.so.11.0: warning: vsprintf() is often misused, please use vsnprintf() basic_radius_auth.o(.text+0x128): In function `main': ../../../../helpers/basic_auth/RADIUS/basic_radius_auth.cc:471: warning: strcpy() is almost always misused, please use strlcpy() /usr/local/lib/gcc/i386-unknown-openbsd4.7/4.2.4/../../../libestdc++.so.11.0: warning: strcat() is almost always misused, please use strlcat() /usr/local/lib/gcc/i386-unknown-openbsd4.7/4.2.4/../../../libestdc++.so.11.0: warning: sprintf() is often misused, please use snprintf() Making all in fake if ccache g++ -DHAVE_CONFIG_H -I../../../.. -I../../../../include -I../../../../src -I../../../include -I/usr/local/include -Wall -Wpointer-arith -Wwrite-strings -Wcomments -Werror -pipe -D_REENTRANT -g -O2 -MT fake.o -MD -MP -MF .deps/fake.Tpo -c -o fake.o ../../../../helpers/basic_auth/fake/fake.cc; then mv -f .deps/fake.Tpo .deps/fake.Po; else rm -f .deps/fake.Tpo; exit 1; fi /bin/sh ../../../libtool --tag=CXX --mode=link ccache g++ -Wall -Wpointer-arith -Wwrite-strings -Wcomments -Werror -pipe -D_REENTRANT -g -O2 -g -o basic_fake_auth fake.o -L../../../lib -lmiscutil ../../../compat/libcompat.la libtool: link: ccache g++ -Wall -Wpointer-arith -Wwrite-strings -Wcomments -Werror -pipe -D_REENTRANT -g -O2 -g -o basic_fake_auth fake.o -Lhttp://build.squid-cache.org/job/3.HEAD-i386-OpenBSD/ws/btlayer-00-default/squid-3.HEAD-BZR/_build/lib -lmiscutil ../../../compat/.libs/libcompat.a /usr/local/lib/gcc/i386-unknown-openbsd4.7/4.2.4/../../../libestdc++.so.11.0: warning: vsprintf() is often misused, please use vsnprintf() /usr/local/lib/gcc/i386-unknown-openbsd4.7/4.2.4/../../../libestdc++.so.11.0: warning: strcpy() is almost always misused, please use strlcpy() /usr/local/lib/gcc/i386-unknown-openbsd4.7/4.2.4/../../../libestdc++.so.11.0: warning: strcat() is almost always misused, please use strlcat() /usr/local/lib/gcc/i386-unknown-openbsd4.7/4.2.4/../../../libestdc++.so.11.0: warning: sprintf() is often misused, please use snprintf() Making all in getpwnam if ccache g++ -DHAVE_CONFIG_H -I../../../.. -I../../../../include -I../../../../src -I../../../include -I/usr/local/include -Wall -Wpointer-arith -Wwrite-strings -Wcomments -Werror -pipe -D_REENTRANT -g -O2 -MT basic_getpwnam_auth.o -MD -MP -MF .deps/basic_getpwnam_auth.Tpo -c -o basic_getpwnam_auth.o ../../../../helpers/basic_auth/getpwnam/basic_getpwnam_auth.cc; then mv -f .deps/basic_getpwnam_auth.Tpo .deps/basic_getpwnam_auth.Po; else rm -f .deps/basic_getpwnam_auth.Tpo; exit 1; fi /bin/sh
