On Thu, Apr 21, 2016 at 02:25:28PM -0400, Jan Harkes wrote:
> On Thu, Apr 21, 2016 at 02:16:02PM +0200, u-m...@aetey.se wrote:
> > The only problem we hit there was not with clog but with memory alignment
> > in cfs (which we then fixed).
>
> Looking forward to seeing patches related to that.
Attaching.
> > There is no http involved (it would be a large overhead without any reason).
> >
> > This data does not have to be secured against MitM, the worst which can
> > happen is disruption of communication / DOS which MitM always can achieve.
>
> Depends on what the configuration contains.
Correct. That's why we can not come to a conclusion without looking at
the actual code / specification. We can return to the subject when we
look at the corresponding patch later.
> > With ViceGetVolumeLocation() we make a similar mistake (!), we do
> > not allow for the returned string to be invalidated "properly". There
> > should have been some kind of a validity promise (say a TTL) included.
> > A possibility to change host names in a realm setup without confusing
> > the clients is a Good Thing (TM), why forbid this by design?
>
> GetVolumeLocation returns a DNS hostname, DNS records have a TTL. There
> is nothing here forbidden by design, we just don't duplicate something
> that is already there.
In your scheme there are several mappings to do:
volume/server -[1]-> dnsname -[2]-> ip
It was not about [2] but about [1] which "risks to fail to become invalidated"
when it is stale.
I understand now that you implied to reresolve volume/server->dnsname
when the client loses contact with a server.
We in our code trigger reresolution when a server becomes unreachable
(for "too long").
Then indeed no special expiration time is needed.
> It just seems like we're having the same discussions over and over
You are right, we have already presented the arguments a couple
of years ago and also commented on them, no need to duplicate.
> again, and yet it always results in to us disagreeing and you just
No, not _that_ bad. We actually agree on most things, we just put the
discussion effort into the ones where we do not :) the other ones go
unnoticed.
Think also, if I wouldn't value your comments, I wouldn't ask for them.
> doing your thing anyway,
That's how the things get done. One implements what one feels is the
most suitable approach.
> Jan
Best regards,
Rune
coda-src/vtools/cfs.cc | 27 +++++++++++++++++++--------
1 files changed, 19 insertions(+), 8 deletions(-)
--- a/coda-src/vtools/cfs.cc
+++ b/coda-src/vtools/cfs.cc
@@ -1826,8 +1826,8 @@ static void ListVolume(int argc, char *argv[], int opslot)
VolumeStatus *vs;
char *volname, *omsg, *motd;
VolumeStateType conn_state;
- int conflict, cml_count;
- unsigned int age, time;
+ int32_t conflict, cml_count;
+ uint32_t age, time;
uint64_t cml_bytes;
char *ptr;
int local_only = 0;
@@ -1859,25 +1859,36 @@ static void ListVolume(int argc, char *argv[], int
opslot)
cml_count, offlinemsg, motd, age, time, cml_bytes) */
ptr = piobuf; /* invariant: ptr always point to next obj
to be read */
+/* we hope that piobuf is sufficiently well aligned */
vs = (VolumeStatus *)ptr;
ptr += sizeof(VolumeStatus);
volname = ptr; ptr += strlen(volname)+1;
- conn_state = (VolumeStateType)*(int32_t *)ptr;
+/* avoid unaligned memory accesses */
+ { int32_t temp;
+ memcpy(&temp, ptr, sizeof(int32_t));
+ conn_state = (VolumeStateType)temp;
+ }
+/* conn_state = (VolumeStateType)*(int32_t *)ptr; */
ptr += sizeof(int32_t);
- conflict = *(int32_t *)ptr;
+ memcpy(&conflict, ptr, sizeof(int32_t));
+/* conflict = *(int32_t *)ptr; */
ptr += sizeof(int32_t);
- cml_count = *(int32_t *)ptr;
+ memcpy(&cml_count, ptr, sizeof(int32_t));
+/* cml_count = *(int32_t *)ptr; */
ptr += sizeof(int32_t);
omsg = ptr; ptr += strlen(omsg)+1;
motd = ptr; ptr += strlen(motd)+1;
- age = *(uint32_t *)ptr;
+ memcpy(&age, ptr, sizeof(uint32_t));
+/* age = *(uint32_t *)ptr; */
ptr += sizeof(uint32_t);
- time = *(uint32_t *)ptr;
+ memcpy(&time, ptr, sizeof(uint32_t));
+/* time = *(uint32_t *)ptr; */
ptr += sizeof(uint32_t);
- cml_bytes = *(uint64_t *)ptr;
+ memcpy(&cml_bytes, ptr, sizeof(uint64_t));
+/* cml_bytes = *(uint64_t *)ptr; */
ptr += sizeof(uint64_t);
/* Print output fields */
