[Cluster-devel] [PATCH 8/8] libgfs2: Add support for rg_crc

[Bob Peterson <rpeterso@redhat.com>]

Hi,

----- Original Message -----
| This new field in the resource group headers is set on write and checked
| on read so that fsck.gfs2 now considers a resource group with a bad,
| non-zero crc to be corrupted. gfs2_grow, gfs2_edit, and mkfs.gfs2 all
| now support the rg_crc field, including save/restoremeta.
| 
| Signed-off-by: Andrew Price <anprice@redhat.com>
| ---
(snip)
| diff --git a/gfs2/libgfs2/rgrp.c b/gfs2/libgfs2/rgrp.c
| index 17178bd9..03861d14 100644
| --- a/gfs2/libgfs2/rgrp.c
| +++ b/gfs2/libgfs2/rgrp.c
| @@ -158,6 +158,42 @@ void lgfs2_rgrp_bitbuf_free(lgfs2_rgrp_t rg)
|  }
|  
|  /**
| + * Check a resource group's crc
| + * Returns 0 on success, non-zero if crc is bad
| + */
| +int lgfs2_rgrp_crc_check(const struct gfs2_rgrp *rg, char *buf)
| +{
| +	int ret = 0;
| +#ifdef GFS2_HAS_RG_RI_FIELDS
| +	uint32_t crc = rg->rg_crc;
| +
| +	if (crc == 0)
| +		return 0;
| +
| +	((struct gfs2_rgrp *)buf)->rg_crc = 0;
| +	if (crc != gfs2_disk_hash(buf, sizeof(struct gfs2_rgrp)))
| +		ret = 1;
| +	((struct gfs2_rgrp *)buf)->rg_crc = cpu_to_be32(crc);

I didn't trace the code all the way back, so I might be wrong. But:

My concern here is that lgfs2_rgrp_crc_check can modify the contents
of the buffer here. That is exactly what you want in most cases, but
it might be a problem for fsck.gfs2 cases other than -y. For example,
The -n option instructs fsck to not make any changes, and without -y,
it's supposed to ask before making any changes. So hopefully this
buffer is never rewritten without permission, at some other point in
fsck. It's worth checking. It also may be worth testing to make sure
specifying -n detects crc errors, reports them, but does not fix them
with -n.

(snip)
| +/**
| + * Set the crc of an on-disk resource group
| + */
| +void lgfs2_rgrp_crc_set(char *buf)
| +{
| +#ifdef GFS2_HAS_RG_RI_FIELDS
| +	struct gfs2_rgrp *rg = (struct gfs2_rgrp *)buf;
| +	uint32_t crc;
| +
| +	rg->rg_crc = 0;
| +	crc = gfs2_disk_hash(buf, sizeof(struct gfs2_rgrp));
| +	rg->rg_crc = cpu_to_be32(crc);
| +#endif
| +}

You may want to move the initialization of rg_crc (and supporting
code, obviously) outside the #ifdef so it gets initialized to 0.

The rest looked good.

Bob Peterson