From mboxrd@z Thu Jan  1 00:00:00 1970
From: Andrew Price <anprice@redhat.com>
Date: Tue, 02 Jul 2013 14:09:08 +0100
Subject: [Cluster-devel] gfs2-utils: master - mkfs.gfs2: Move the new
 rgrp creation code into libgfs2
In-Reply-To: <1983072178.106607948.1372768746625.JavaMail.root@redhat.com>
References: <20130702092636.3ECC760DED@fedorahosted.org>
	<1983072178.106607948.1372768746625.JavaMail.root@redhat.com>
Message-ID: <51D2D0F4.8010007@redhat.com>
List-Id: <cluster-devel.redhat.com>
To: cluster-devel.redhat.com
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit

On 02/07/13 13:39, Bob Peterson wrote:
> Hi,
>
> Comments embedded.
>
> ----- Original Message -----
> | mkfs.gfs2: Move the new rgrp creation code into libgfs2
> (snip)
> | +// Temporary function to aid in API migration
>
> This ^ is a C++ comment, and should be: /* ... */

The comment style was added to C in C99 and we use -std=gnu99 already so 
it shouldn't cause any problems. It'll get ripped out with the temporary 
function eventually.

> | + * rglen: The required length of the resource group. If its is 0 the default
>
> "If its is 0" should be "If it is 0" (Sorry: Grammar Nazi at work here. :) )

 From one grammar Nazi to another: well-spotted, thanks :)

> | +	rg->ri.ri_length = rgblocks2bitblocks(rgs->bsize, rglen, &rg->ri.ri_data);
> | +	rg->ri.ri_data0 = rg->ri.ri_addr + rg->ri.ri_length;
> | +	rg->ri.ri_bitbytes = rg->ri.ri_data / GFS2_NBBY;
> | +	rg->rg.rg_header.mh_magic = GFS2_MAGIC;
> | +	rg->rg.rg_header.mh_type = GFS2_METATYPE_RG;
> | +	rg->rg.rg_header.mh_format = GFS2_FORMAT_RG;
>
> I don't know if this memory gets zeroed out, but perhaps we should explicitly
> zero out rg->rg.rg_header.__pad0 as well. There have been times when we reused
> unused fields like this for things like generation numbers (mainly for
> debugging purposes) so I'd hate to have this value uninitialized. If the struct
> is zeroed out, don't worry about it.

Yep, it gets zeroed in rgrp_insert.

>
> | +/**
> | + * Write a resource group to a file descriptor.
> | + * Returns 0 on success or non-zero on failure with errno set
> | + */
> | +int lgfs2_rgrp_write(int fd, lgfs2_rgrp_t rg, unsigned bsize)
> | +{
> | +	ssize_t ret = 0;
> | +	size_t len = rg->ri.ri_length * bsize;
> | +	unsigned int i;
> | +	const struct gfs2_meta_header bmh = {
> | +		.mh_magic = GFS2_MAGIC,
> | +		.mh_type = GFS2_METATYPE_RB,
> | +		.mh_format = GFS2_FORMAT_RB,
>
> Here again, maybe we should set __pad0 explicitly rather than leaving the
> value uninitialized.

Looks like Steve's replied to this bit already.

Thanks for reviewing,

Andy

>
> The rest looks good to me.
>
> Regards,
>
> Bob Peterson
> Red Hat File Systems
>