Details
-
Bug
-
Resolution: Duplicate
-
Major
-
None
-
None
-
None
-
3
-
9942
Description
Extensions to liblustreapi have been posted for review here.
http://review.whamcloud.com/5302
The goal of the API extensions is to provided a user-friendly interface for interacting with the layout of files in Lustre filesystems, and to hide the wire-protocol details behind an opaque data type.
This issue will provide a forum for potentials users and developers to discuss and critique the design of the proposed API extensions.
Attachments
Issue Links
Activity
I am all for abandoning the old api as soon as possible. It is a maintenance nightmare for my applicaiton code to handle. There is ugly and otherwise unneeded ifdef goo for different machines all over the place. Please dump that api. What Ned says is exactly right: there is a chance to do it right/better this time around.
As far as "negative value returned" vs "specific negative value returned (i.e. rc=-ETHE_ERROR_CODE), again, this leads to confusion and complexity that is just not needed. What happens when someone makes a commit sometime and gets them out of sync, such that the return code and errno are not the same any more, for example? Yes, thats a bug that
someone introduced, but that is the whole point. You can remove that bug years ahead of time by making it impossible to do that by design.
so as it is clearly documented in the man pages that the functions return a negative value on error instead of "-1" there shouldn't be any problem.
If the man page merely specifies a negative value on error, than a properly written application shouldn't rely on the specific value returned. Therefore in order for this behavior to be useful, it most become a formal part of the API and documented as such. Simplicity is arguably the most important aspect of a well-designed API, and having two alternative means of returning error codes would add complexity for little gain. I'm not persuaded we should do it just because the legacy llapi functions do it. We should see this as an opportunity to get things right this time, not to carry over all the old baggage.
Because it would keep the same API that the rest of liblustreapi has today, and there is no drawback to doing so. Virtually every application I've seen checks if (rc < 0) instead of if (rc == -1), so as it is clearly documented in the man pages that the functions return a negative value on error instead of "-1" there shouldn't be any problem. That allows applications to choose which behaviour they want to use for programming, and I don't see it introducing any significant complexity into the library - at worst it would mean return -errno instead of return -1 in some places.
If there is an insistence on setting errno to return errors, it would still be possible to also return the negative errno instead of "-1" all the time.
We could, but what would be the benefit? It would be awkward and potentially confusing for the API to specify more than one authoritative source of error codes. And as Andy points out, applications using both standard library and llapi_layout calls would prefer to use common error handling constructs.
FWIW, I have already implemented the version of the api that uses errno stuff, and put strerror calls in various
error paths. For example:
rc = llapi_layout_stripe_count_get(layout,&num_comps ); if(rc!=0){*ierr=-1;goto writeerrout;};
...
writeerrout:
sprintf(cerr,"Lustre layout definition error: %s\n",strerror(errno));
This api is consistent with how I do things in other parts of the code as well, e.g. with stat calls and such.
As an implementer of userland code that uses the llapi functionality, I strongly prefer the errno approach.
If there is an insistence on setting errno to return errors, it would still be possible to also return the negative errno instead of "-1" all the time. I'm not suggesting to return PTR_ERR() instead of NULL in case of memory allocation failures, as I agree that this is not very common for userspace programs.
IMHO, "errno" is the domain of the kernel and libc and should not be used by application libraries.
I think that the lustre library should be considered a system library, not an "application library". From a normal application's perspective, the lustre library is as system as they come: you are using library the interacts on your behalf directly with the kernel to influence a service offered by the kernel. In that respect I would argue that errno is entirely reasonable to use.
I would argue that this kind of error handling is exactly what user-space C developers have come to expect from system level libraries. After all, if it isn't OK for use to use errno, is it really OK for us to reuse all of the standard error codes (EIO, EINVAL, etc.)? Shouldn't we have to invent our own error names and values if those things are only the purview of the kernel and Lib C?
Granted, using temporary variable to implement the use of error is mildly annoying. But is that really enough justification to violate the principle of least surprise for the user-space developers who will be consuming our library functions?
I think that the "Return negated errno values" approach is probably the least desirable of those proposed. This is a kernel-ism; the result of a clever hack that recognized that those memory values would never be valid so hey why not throw the error value in there. In user space, the programmers are going to think we have lost our minds if we force them to check for an negative version of an error code that is always positive everywhere else. At the very least, we would need to create macros or functions that the users would need to use to check the return code and another to translate the error code into the correct value. We would be shifting the annoying error code shuffling from the library writer to all of the library consumers.
errno is defined by the C language standard. Most, if not all, of the values of errno that we use (EACCESS, EAGAIN, EIO, EISDIR, etc.) are defined by POSIX.1-2001 or C99, not inventions of the Linux kernel.
If we use the same values as errno, users are going to want to use standard functions like perror() that assume the use of errno. Granted, strerror() also exists, but it is more difficult to use than perror().
It is already difficult to get users to check error codes, so I think it is important to keep things simple when reasonable to do so.
I'm on board with this. I just had a hallway discussion about the various approaches for returning errors, and negative errno return values were generally agreed to be the least evil of the following possibilities:
- Use errno.
This is the current approach. It's evil for the reasons given above by Andreas. - Use a library-specific version of errno.
e.g. llapi_errno. It wouldn't get stomped on, but we'd have to handle thread safety. Ick. - Implement our own class of error codes.
This might be cleaner and more flexibile, but with a higher implementation and maintenance cost, plus UNIX programmers will be already familiar errno values. - Return negated errno values.
This is the proposed approach. The only real downside is there's little precedent outside the kernel and llapi. But it's thread safe and cleaner than the current approach.
One thing that snuck past my review in these patches was that the new llapi_layout_*() functions are all returning "-1" to the caller and returning the error codes via "errno" instead of returning the negative error numbers directly to the callers. IMHO, "errno" is the domain of the kernel and libc and should not be used by application libraries. This is a global variable that could be touched by many parts of the process, and there is the danger that errno gets clobbered by other parts of the code, leading to ugliness like:
if (lum == NULL) { tmp = errno; close(fd); errno = tmp; return -1; }
and every piece of code that is returning an error having to do it twice:
if (path == NULL || (layout != NULL && layout->llot_magic != LLAPI_LAYOUT_MAGIC)) { errno = EINVAL; return -1; }
My preference would be to fix the new llapi_layout_*() functions to return the negative error number directly and avoid errno entirely.
Updated attached man pages to reflect recent API changes. In particular
- removed llapi_layout_expected()
- llapi_layout_by_{fd,fid,path}() renamed to llapi_layout_get_by_{fd,fid,path}()
- Added flags parameter to llapi_layout_get_by_{fd,fid,path}()
- Implemented flag LAYOUT_GET_EXPECTED which is accepted by llapi_layout_get_by_path() to implement functionality formerly provided by llapi_layout_expected()
Please review the documentation of the new flags parameter in llapi_layout_get_by_fd.txt
.
Andy, while I'm all for updating applications to the new API, yours is not the only application in the world that uses it, so we can't remove the old API very quickly. Also, this new API has only just been landed into a development branch and would need to be backported into the maintenance releases before it even has a chance to be used by regular users.
I think the best that can be done is to include this into all of the maintenance releases, update the old APIs to use this new code (James has started on that) and then it can be deprecated in a few years after it is available in those releases. It is also possible to mark those functions as deprecated in the headers so that application developers learn this before the API is removed. It would also make sense to update the user manual once this API is available in the maintenance releases.