[PATCH] (7/11) API improvements

[PATCH] (7/11) API improvements

[Petr Rockai]

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

-- 
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 indentation

[PATCH] (7/11) API improvements

[Dave Wysochanski]

On Thu, 2008-10-30 at 19:15 +0100, Petr Rockai wrote:
>         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);
> -                       vg->failed |= FAILED_INCONSISTENT;
> +                       failure |= FAILED_INCONSISTENT;
>                         goto_bad;
>                 }
>                 if (!(misc_flags & EXISTENCE_CHECK))
>                         log_error("Volume group \"%s\" not found",
> vg_name);
> -               vg->failed |= FAILED_NOTFOUND;
> +               failure |= FAILED_NOTFOUND | (misc_flags &
> EXISTENCE_CHECK);
>                 goto_bad;
>         }
>  
> -       vg->failed |= _vg_check_status(vg, status_flags);
> +       /* consistent == 0 when VG is not found, but failed ==
> FAILED_NOTFOUND */
> +       if (!consistent && !vg->read_failed) {

I believe this should be:
	if (!consistent && !failure) {


> +               log_error("Volume group \"%s\" inconsistent.",
> vg_name);
> +               failure |= FAILED_INCONSISTENT;
> +               goto_bad;
> +       }
>  
> 

[PATCH] (7/11) API improvements

[Petr Rockai]

Dave Wysochanski <dwysocha@redhat.com> writes:
> 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.

-- 
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 indentation

[PATCH] (7/11) API improvements

[Dave Wysochanski]

On Thu, 2008-10-30 at 19:15 +0100, Petr Rockai wrote:
> +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);
> +       return vg->read_failed;
> +}
> +
> +/* Returns true if the volume group already exists. If unsure, it
> will return
> +   true (it might exist, but we are not sure, as the read failed for
> some
> +   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);
> +       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] (7/11) API improvements

[Petr Rockai]

Dave Wysochanski <dwysocha@redhat.com> writes:
> 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.

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.

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

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.

-- 
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 indentation

[PATCH] (7/11) API improvements

[Dave Wysochanski]

On Mon, 2008-11-24 at 16:56 +0100, Petr Rockai wrote:
> Dave Wysochanski <dwysocha@redhat.com> writes:
> > 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.
> 
> 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.
> 
> > 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.
> 
> 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.
> 
You are right there - it is not good to assume API call sequence for
correctness.


> So if it was me, I'd keep that interface...
> 

Ok, let me think some more about this and the error code stuff.