[lvm-devel] [PATCH 2/11] Provide _vg_lock_and_read, a new implementation of lock+vg_read_internal.

[Dave Wysochanski]

On Thu, 2009-01-15 at 10:25 +0100, Petr Rockai wrote:
> Dave Wysochanski <dwysocha at redhat.com> writes:
> > Note only FAILED_READ_ONLY is used by any tool today - only vgreduce.
> > Further, FAILED_READ_ONLY can only occur with LVM1 metadata as far as I
> > can tell.
> Could be. I'd propose to address this separately though, since I suppose we
> want to change semantics of things for this -- make the read-only failure
> deprecated and maybe just issue a warning whenever the flag is encountered,
> when opening read-write?
> 

Fine to address separately - let's make a note of it.

> >> +	if (!validate_name(vg_name) && !is_orphan_vg(vg_name)) {
> >> +		log_error("Volume group name %s has invalid characters",
> >> +			  vg_name);
> >> +		return NULL;
> >> +	}
> >
> > Is there a reason to return NULL here when a handle with an error code
> > is returned everywhere else?
> >
> > Note that callling vg_read_error() after hitting this path will return
> > FAILED_ALLOCATION.  Do we need FAILED_INVALID_NAME or perhaps re-use
> > FAILED_NOT_FOUND?
> Basically, the reason is just that no tool ever seems to care. It's true that
> it's not very nice to turn up FAILED_ALLOCATION, but in case that's what API
> user gets, they should just abort anyway -- the library code has already
> printed the error message (like it is the case here). We could differentiate
> that though, I have no strong opinion. Maybe add a patch on top though.
> 

Fine with me to clean up later.  I would imagine we'll have some error
code refinement to do anyway.