[Ocfs2-tools-devel] [PATCH 07/11] Ocfs2-tools: Add implementation of operations to o2info.

[Joel Becker]

On Fri, Jan 08, 2010 at 04:50:57PM +0800, Tristan Ye wrote:
> All of o2info's operations will be linked to a task list,
> and executed one by one later, the real running code of
> a new item for o2info is expected to be added there.
> 
> Signed-off-by: Tristan Ye <tristan.ye at oracle.com>
> ---
>  o2info/o2info_operations.c | 1198 ++++++++++++++++++++++++++++++++++++++++++++

	Stop calling them 'o2info_operations.c', etc.  It's a pain to
type, and we know what program it belongs to because it is in the o2info
directory.  mv o2info/o2info_operations.c o2info/operations.c.

> +static inline void __o2info_fill_request_header(struct ocfs2_info_request *req,
> +						size_t size, int code,
> +						int flags)
> +{
> +	req->ir_magic = OCFS2_INFO_MAGIC;
> +	req->ir_code = code;
> +	req->ir_flags = flags;
> +	req->ir_size = size;
> +}
> +
> +#define o2info_fill_request(req, code, flags)		do {		\
> +	memset(req, 0, sizeof(*req));					\
> +	__o2info_fill_request_header((struct ocfs2_info_request *)req,	\
> +				     sizeof(*req), code, flags);	\
> +} while (0)

	Comment that you use a #define so that sizeof() acts on the full
request type.

> +static void o2info_fprintf(struct o2info_operation *op,
> +			   FILE *stream, const char *fmt, ...)
> +{
> +	va_list ap;
> +
> +	fprintf(stream, "%s: ", op->to_name);
> +	va_start(ap, fmt);
> +	vfprintf(stream, fmt, ap);
> +
> +	return;
> +}

	You only use this for errors as far as I can see.  Just call it
o2i_error().  Don't pass a FILE*, just hardcode stderr inside the
function.
	Break this patch up.  The first patch adds all the helpers.
Then one patch each per operation.

> +static int get_volinfo(struct o2info_operation *op, struct o2info_method *om,
> +		       struct o2info_volinfo *vf)
> +{
> +	int rc = 0;
> +	struct ocfs2_super_block *sb = NULL;
> +
> +	memset(vf, 0, sizeof(struct o2info_volinfo));
> +
> +	if (om->om_method & O2INFO_USE_LIBOCFS2) {
> +
> +		sb = OCFS2_RAW_SB(om->om_fs->fs_super);
> +		vf->blocksize = om->om_fs->fs_blocksize;
> +		vf->clustersize = om->om_fs->fs_clustersize;
> +		vf->slotnum = sb->s_max_slots;
> +		memcpy(vf->label, sb->s_label, OCFS2_INFO_MAX_VOL_LABEL_LEN);
> +		memcpy(vf->uuid, sb->s_uuid, OCFS2_INFO_VOL_UUID_LEN);
> +		goto out;
> +	}
> +
> +	if (om->om_method & O2INFO_USE_IOCTL) {
> +
> +		struct ocfs2_info_blocksize brq;
> +		struct ocfs2_info_clustersize crq;
> +		struct ocfs2_info_slotnum srq;
> +		struct ocfs2_info_label lrq;
> +		struct ocfs2_info_uuid urq;
> +
> +		o2info_fill_request(&brq, OCFS2_INFO_BLOCKSIZE, no_coherency);
> +		o2info_fill_request(&crq, OCFS2_INFO_CLUSTERSIZE, no_coherency);
> +		o2info_fill_request(&srq, OCFS2_INFO_SLOTNUM, no_coherency);
> +		o2info_fill_request(&lrq, OCFS2_INFO_LABEL, no_coherency);
> +		o2info_fill_request(&urq, OCFS2_INFO_UUID, no_coherency);
> +
> +		uint64_t reqs[5] = {(unsigned long)&brq,
> +				    (unsigned long)&crq,
> +				    (unsigned long)&srq,
> +				    (unsigned long)&lrq,
> +				    (unsigned long)&urq};
> +
> +		struct ocfs2_info info = {
> +			.info_requests = (uint64_t)reqs,
> +			.info_count = 5,
> +		};
> +
> +		rc = ioctl(om->om_fd, OCFS2_IOC_INFO, &info);
> +		if (rc) {
> +			rc = errno;
> +			o2info_fprintf(op, stderr, "ioctl failed:%d %s\n",
> +				       rc, strerror(rc));
> +			goto out;
> +		}
> +
> +		if (brq.ir_request.ir_flags & OCFS2_INFO_FL_FILLED)
> +			vf->blocksize = brq.ir_blocksize;
> +
> +		if (crq.ir_request.ir_flags & OCFS2_INFO_FL_FILLED)
> +			vf->clustersize = crq.ir_clustersize;
> +
> +		if (srq.ir_request.ir_flags & OCFS2_INFO_FL_FILLED)
> +			vf->slotnum = srq.ir_slotnum;
> +
> +		if (lrq.ir_request.ir_flags & OCFS2_INFO_FL_FILLED)
> +			memcpy(vf->label, lrq.ir_label,
> +			       OCFS2_INFO_MAX_VOL_LABEL_LEN);
> +
> +		if (urq.ir_request.ir_flags & OCFS2_INFO_FL_FILLED)
> +			memcpy(vf->uuid_str, urq.ir_uuid_str,
> +			       OCFS2_INFO_VOL_UUIDSTR_LEN);
> +
> +		goto out;
> +	}
> +
> +out:
> +	return rc;
> +}

	For each operation, separate out the different methods as
subfunctions.  Rather than this large function above, have:

    o2i_get_volinfo_lib(...)
    {
        ...
    }

    o2i_get_volinfo_ioctl(...)
    {
        ...
    }

    o2i_get_volinfo(...)
    {
        if (method == O2INFO_USE_IOCTL)
            return o2i_get_volinfo_ioctl()
        else
            return o2i_get_volinfo_lib()
    }

	Note also that I use '== O2INFO_USE_IOCTL', not '&'.  It's not a
bit, it's an integer.

> +		rc = ioctl(om->om_fd, OCFS2_IOC_INFO, &info);
> +		if (rc) {
> +			rc = errno;
> +			o2info_fprintf(op, stderr, "ioctl failed:%d %s\n",
> +				       rc, strerror(rc));

	You don't need to print errno in these instances.  Just print
the result of strerror().  The same for any errors printing errno.

Joel

-- 

Life's Little Instruction Book #450

	"Don't be afraid to say, 'I need help.'"

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127