(7/11) API improvements
PATCH 1/1 (7/11) API improvements
Hi,
this patch improves the API somewhat, by restructuring the way errors are checked for by the client code: a vg_read_error function is provided for that, and the vg_read(_for_update) has seen some improvements unrelated to that. It also makes vg_read_for_update a simple front-end to vg_read, as vg_read will be needed later to have the same capabilities, easily (this is done through the READ_FOR_UPDATE flag). I admit that this is a little incoherent together in a single patch, but that's how things winded up.
Yours,
Petr.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: lvm2-lvmlib-newapi.diff
Type: text/x-diff
Size: 20708 bytes
Desc: lvmlib-new-api.diff
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20081030/ea88f20a/attachment.bin>
-------------- next part --------------
Signature · 6 lines
Peter Rockai | me()mornfall!net | prockai()redhat!com
http://blog.mornfall.net | http://web.mornfall.net
"In My Egotistical Opinion, most people's C programs should be
indented six feet downward and covered with dirt."
-- Blair P. Houghton on the subject of C program indentationPATCH 1/1 (7/11) API improvements
Quoted 21 lines · Petr Rockai
if (!(vg =
vg_read_internal(cmd, vg_name, vgid, &consistent)))
{
- vg =_vg_read_failure(cmd, 0);
if (consistent_in&& !consistent) {
log_error("Volume group \"%s\" inconsistent.",vg_name);(quoted diff) +1
-1- vg->failed |= FAILED_INCONSISTENT; + failure |= FAILED_INCONSISTENT; goto_bad; } if (!(misc_flags & EXISTENCE_CHECK)) log_error("Volume group \"%s\" not found",
vg_name);(quoted diff) +1
-1- vg->failed |= FAILED_NOTFOUND; + failure |= FAILED_NOTFOUND | (misc_flags &EXISTENCE_CHECK); goto_bad; }(quoted diff) +1
-1- vg->failed |= _vg_check_status(vg, status_flags); + /* consistent == 0 when VG is not found, but failed ==
Quoted 2 lines
FAILED_NOTFOUND*/
+ if (!consistent && !vg->read_failed) {
I believe this should be:
if (!consistent && !failure) {Quoted 7 lines
+ log_error("Volume group \"%s\" inconsistent.",
vg_name);(quoted diff) +3
-0+ failure |= FAILED_INCONSISTENT; + goto_bad; + }
PATCH 1/1 (7/11) API improvements
Quoted 2 lines · Dave Wysochanski
I believe this should be:
if (!consistent && !failure) {
Indeed, and that's an actual bug. Noting it down. (Where I write "note down" I mean that I fix it on my branch, so it'll be fixed in the final commit.)
Yours,
Petr.
Signature · 6 lines
Peter Rockai | me()mornfall!net | prockai()redhat!com
http://blog.mornfall.net | http://web.mornfall.net
"In My Egotistical Opinion, most people's C programs should be
indented six feet downward and covered with dirt."
-- Blair P. Houghton on the subject of C program indentationPATCH 1/1 (7/11) API improvements
In reply to Petr RockaiQuoted 21 lines · Petr Rockai
(quoted diff) +5
-0+int vg_read_error(vg_t *vg) { + if (!vg) + return FAILED_ALLOCATION; + if (vg->read_failed & EXISTENCE_CHECK) + return vg->read_failed & ~(EXISTENCE_CHECK |
FAILED_NOTFOUND);(quoted diff) +4
-0+ return vg->read_failed; +} + +/* Returns true if the volume group already exists. If unsure, itwill return
+ true (it might exist, but we are not sure, as the read failed for
some(quoted diff) +6
-0+ other reason). */ + +int vg_exists(vg_t *vg) { + if (!vg) + return 1; + log_error("vg_exists: %d != %d?", vg->read_failed,
FAILED_NOTFOUND|EXISTENCE_CHECK);
Review
Quoted 4 lines
(quoted diff) +4
-0+ if (vg->read_failed == (FAILED_NOTFOUND | EXISTENCE_CHECK)) + return 0; + return 1; +}
Although the comment explains the vg_exists() function may be just a guess, it is misleading to have a function called "vg_exists()" that takes a vg_t * and returns true of vg_t * is NULL.
Also from what I can tell, all places that call vg_exists() is preceeded immediately by a call to vg_read_error() which does the same check.
PATCH 1/1 (7/11) API improvements
Quoted 3 lines · Dave Wysochanski
Although the comment explains the
vg_exists()function may be just a guess, it is misleading to have a function called "vg_exists()" that takes avg_t* and returns true ofvg_t* is NULL.
It might be counterintuitive, but that's one of the reasons the function needs to exist. It's pretty likely that if people would try to implement that check themselves, they'd get it wrong.
Quoted 2 lines
Also from what I can tell, all places that call
vg_exists()is preceeded immediately by a call tovg_read_error()which does the same check.
Yes, but you can't rely on that when designing an API. Requiring that vg_read_error() is checked before using vg_exists() is counterintuitive as well, and counterintuitive API is worse than counterintuitive implementation of certain details in it. Or so I think.
So if it was me, I'd keep that interface...
Yours,
Petr.
Signature · 6 lines
Peter Rockai | me()mornfall!net | prockai()redhat!com
http://blog.mornfall.net | http://web.mornfall.net
"In My Egotistical Opinion, most people's C programs should be
indented six feet downward and covered with dirt."
-- Blair P. Houghton on the subject of C program indentationPATCH 1/1 (7/11) API improvements
Quoted 9 lines · Petr Rockai
Quoted 3 lines · Dave Wysochanski
Although the comment explains the
vg_exists()function may be just a guess, it is misleading to have a function called "vg_exists()" that takes avg_t* and returns true ofvg_t* is NULL.It might be counterintuitive, but that's one of the reasons the function needs to exist. It's pretty likely that if people would try to implement that check themselves, they'd get it wrong.
Quoted 8 lines
Quoted 2 lines
Also from what I can tell, all places that call
vg_exists()is preceeded immediately by a call tovg_read_error()which does the same check.Yes, but you can't rely on that when designing an API. Requiring that
vg_read_error()is checked before usingvg_exists()is counterintuitive as well, and counterintuitive API is worse than counterintuitive implementation of certain details in it. Or so I think.
You are right there - it is not good to assume API call sequence for correctness.
Quoted 2 lines
So if it was me, I'd keep that interface...
Ok, let me think some more about this and the error code stuff.