[Ocfs2-tools-devel] [PATCH 4/6] O2info: Add running codes for '--fs-features'.

[tristan]

Joel Becker wrote:
> On Fri, Apr 23, 2010 at 03:34:53PM -0700, Sunil Mushran wrote:
>   
>>> +	rc = ioctl(fd, OCFS2_IOC_INFO, &info);
>>> +	if (rc) {
>>> +		rc = errno;
>>> +		o2i_error(op, "ioctl failed: %s\n", strerror(rc));
>>> +		goto out;
>>> +	}
>>> +
>>> +	if (oif.if_req.ir_flags & OCFS2_INFO_FL_FILLED) {
>>> +		ofs->compat = oif.if_compat_features;
>>> +		ofs->incompat = oif.if_incompat_features;
>>> +		ofs->rocompat = oif.if_ro_compat_features;
>>> +	}
>>> +
>>> +out:
>>> +	return rc;
>>> +}
>>>   
>>>       
>> What happens if rc == 0 and flags is not FILLED. Best if
>> we ensure that cannot happen. As in, kernel should return
>> an error if it is not filled.
>>     
>
> 	He's correct here.  Remember, we allow !FILLED for forwards
> compatibility.  The kernel skips requests it doesn't understand rather
> than returning an error.  So if you pass N requests, you have to check
> each one to see if FILLED was set to determine whether the kernel
> understood it.  The 0 just means there was no actual error.
> 	But this brings up a good point.  As currently defined, we
> return our first error.  Imagine that we fill the first six requests out
> of an info_count of eight.  Then the seventh request has a bad pointer.
> Currently we return -EFAULT, and the caller misses the six finished
> requests.
> 	I can think of three solutions.  First, we can just leave it
> as-is - if there is any error, you get an error and assume nothing is
> filled in.
> 	Next, we could go with read(2) semantics.  Rather than returning
> 0 on success, we return the number of filled requests.  We only return
> an error if we had the error before we filled any request.  Say you have
> two requests.  The first request is successfully filled.  The second has
> a bad pointer.  You return 1.  The caller tries again passing only the
> second request.  Now they get -EFAULT.  Unknown requests are skipped.
> So if you pass in eight requests, and the call returns 4, you only
> need to walk the requests until you've found four that were FILLED.
> 	The biggest downside with this method is the same as for
> read(2): if you get a short count, you have to call again to find out
> why.
>
> --8<-------------------------------------------------------------------
> retry:
> 	rc = ioctl(fd, INFO, &info);
> 	if (rc < 0)
> 		goto error;
> 	assert(rc <= info.info_coutn);
> 	for (i = 0, seen = 0; i < rc; i++) {
> 		if (reqs[i].ir_flags & FILLED) {
> 			/* do something */
> 			seen++;
> 		}
> 	}
> 	/* Did we get to the end */
> 	if ((rc < info.info_count) && (i < info.info_count)) {
> 		info.info_requests = info.info_requests + i;
> 		info.info_count -= info.info_count;
> 		goto retry;
> 	}
> -->8-------------------------------------------------------------------
>
> 	That's pretty clunky for every user to do.  Of course, we can
> wrap in the library somewhat, so that the retry is handled by the
> library car.
> 	Finally, we could do it ASMLib style.  Add req.ir_error and
> OCFS2_INFO_FL_ERROR.  Here, we only return an error from the ioctl(2)
> when the failure was in the ioctl(2) call itself.  So the ioctl(2)
> returns 0 if it exists and the info structure is valid.  Any error on a
> particular request is set in ir_error; this causes FL_ERROR to be set.
>   

Hi Joel,

Here we only set the FL_ERROR to denote whether an error happened or 
not, we're not going to record the type of error, so why bother to add a 
new req.ir_error? can't it be just set in req.ir_flags?


> 	The caller now walks the requests and can determined that
> FL_ERROR is an error, FL_FILLED is completed, and neither means the
> request wasn't supported.
>
> Joel
>
>