[Cluster-devel] gfs2-utils: master - mkfs.gfs2: Move the new rgrp creation code into libgfs2

[Cluster-devel] gfs2-utils: master - mkfs.gfs2: Move the new rgrp creation code into libgfs2

[Bob Peterson]

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: /* ... */

| + * 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. :) )

| +	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.

| +/**
| + * 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.

The rest looks good to me.

Regards,

Bob Peterson
Red Hat File Systems

[Cluster-devel] gfs2-utils: master - mkfs.gfs2: Move the new rgrp creation code into libgfs2

[Steven Whitehouse]

On Tue, 2013-07-02 at 08:39 -0400, Bob Peterson wrote:

> | +/**
> | + * 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.
> 
With this form of initialisation the other fields will all be set to
zero anyway, so it doesn't need to be done explicitly here,

Steve.

[Cluster-devel] gfs2-utils: master - mkfs.gfs2: Move the new rgrp creation code into libgfs2

[Andrew Price]

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
>