* [Cluster-devel] gfs2-utils: master - mkfs.gfs2: Move the new rgrp creation code into libgfs2
2013-07-02 12:39 ` [Cluster-devel] gfs2-utils: master - mkfs.gfs2: Move the new rgrp creation code into libgfs2 Bob Peterson
@ 2013-07-02 12:55 ` Steven Whitehouse
2013-07-02 13:09 ` Andrew Price
1 sibling, 0 replies; 3+ messages in thread
From: Steven Whitehouse @ 2013-07-02 12:55 UTC (permalink / raw)
To: cluster-devel.redhat.com
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.
^ permalink raw reply [flat|nested] 3+ messages in thread* [Cluster-devel] gfs2-utils: master - mkfs.gfs2: Move the new rgrp creation code into libgfs2
2013-07-02 12:39 ` [Cluster-devel] gfs2-utils: master - mkfs.gfs2: Move the new rgrp creation code into libgfs2 Bob Peterson
2013-07-02 12:55 ` Steven Whitehouse
@ 2013-07-02 13:09 ` Andrew Price
1 sibling, 0 replies; 3+ messages in thread
From: Andrew Price @ 2013-07-02 13:09 UTC (permalink / raw)
To: cluster-devel.redhat.com
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
>
^ permalink raw reply [flat|nested] 3+ messages in thread