[Cluster-devel] fixing cpg names in 3.0.8?

[Cluster-devel] fixing cpg names in 3.0.8?

[Christine Caulfield]

On 12/01/10 16:24, David Teigland wrote:
> I just discovered that the cpg group names that fenced, dlm_controld and
> gfs_controld use in cluster3 include a nul within the name length.  This
> can happen because cpg names have both a string part and a length part,
> and the length is currently being set to strlen + 1 instead of strlen.
> It's harmless since all the code is making the same mistake, but it's an
> obvious flaw.
>
> You can easily see this by the trailing \x00 from corosync-cpgtool,
>
> fenced:default\x00
>
> If we fixed the length of the names, it would break internode
> compatibility between 3.0.7 and 3.0.8 releases, e.g.
>
> "fenced:default\x00" and "fenced:default" are two different groups.
>
> AFAIK we're not yet guaranteeing rolling upgrade compatibility from one
> 3.0 release to the next, so I think I should go ahead and fix this, unless
> anyone feels strongly otherwise?


I'm sure we've seen this before. Oh well I'll repeat my opinion anyway.

I don't think it's a flaw and I don't see that it needs to be "fixed". 
It's just the way things happen. What benefits accrue from changing it ?

Chrissie

[Cluster-devel] fixing cpg names in 3.0.8?

[David Teigland]

I just discovered that the cpg group names that fenced, dlm_controld and
gfs_controld use in cluster3 include a nul within the name length.  This
can happen because cpg names have both a string part and a length part,
and the length is currently being set to strlen + 1 instead of strlen.
It's harmless since all the code is making the same mistake, but it's an
obvious flaw.

You can easily see this by the trailing \x00 from corosync-cpgtool,

fenced:default\x00

If we fixed the length of the names, it would break internode
compatibility between 3.0.7 and 3.0.8 releases, e.g.

"fenced:default\x00" and "fenced:default" are two different groups.

AFAIK we're not yet guaranteeing rolling upgrade compatibility from one
3.0 release to the next, so I think I should go ahead and fix this, unless
anyone feels strongly otherwise?

Dave

[Cluster-devel] fixing cpg names in 3.0.8?

[Jan Friesse]

David,

> How about making corosync-cpgtool default to the -e behavior, and making
> -e = escape unprintable characters in group name ?
> 
This can be done quickly, but I don't understand what is that good for
and how does it help. Internally (in cpg exec) names are compared with
full length. This behavior doesn't change from Whitetank. Only thing
what changed was addition of cpgtool, which will notice you on this (in
reality, testcpg, ... will display 0x00 too, but it not visible).

Regards,
  Honza

[Cluster-devel] fixing cpg names in 3.0.8?

[David Teigland]

On Tue, Jan 12, 2010 at 04:00:20PM +0000, Christine Caulfield wrote:
> On 12/01/10 16:24, David Teigland wrote:
> >I just discovered that the cpg group names that fenced, dlm_controld and
> >gfs_controld use in cluster3 include a nul within the name length.  This
> >can happen because cpg names have both a string part and a length part,
> >and the length is currently being set to strlen + 1 instead of strlen.
> >It's harmless since all the code is making the same mistake, but it's an
> >obvious flaw.
> >
> >You can easily see this by the trailing \x00 from corosync-cpgtool,
> >
> >fenced:default\x00
> >
> >If we fixed the length of the names, it would break internode
> >compatibility between 3.0.7 and 3.0.8 releases, e.g.
> >
> >"fenced:default\x00" and "fenced:default" are two different groups.
> >
> >AFAIK we're not yet guaranteeing rolling upgrade compatibility from one
> >3.0 release to the next, so I think I should go ahead and fix this, unless
> >anyone feels strongly otherwise?
> 
> 
> I'm sure we've seen this before. Oh well I'll repeat my opinion anyway.
> 
> I don't think it's a flaw and I don't see that it needs to be "fixed". 
> It's just the way things happen. What benefits accrue from changing it ?

Good point, it isn't a flaw per se, we just have inconsistent usages of
that api (ocfs_controld, at least, sets length to strlen).  There's
something to be said for consistency, but you've convinced me it's not
worth changing.

How about making corosync-cpgtool default to the -e behavior, and making
-e = escape unprintable characters in group name ?

Dave