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

[tristan]

Joel Becker wrote:
> 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.
>   

It makes sense.


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

That will be fine.

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

Ok, I'll do it in later patch series.

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

Yes, It makes sense.

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

Got it.


> Joel
>
>