[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]