[PATCH] Print error if VG already exist.

[PATCH] Print error if VG already exist.

[Milan Broz]

This test have to be moved because of new vg read error handling.

Signed-off-by: Milan Broz <mbroz@redhat.com>
---
 tools/vgcreate.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/tools/vgcreate.c b/tools/vgcreate.c
index b1b0c63..8aef7a2 100644
--- a/tools/vgcreate.c
+++ b/tools/vgcreate.c
@@ -55,8 +55,13 @@ int vgcreate(struct cmd_context *cmd, int argc, char **argv)
 
 	/* Create the new VG */
 	vg = vg_create(cmd, vp_new.vg_name);
-	if (vg_read_error(vg))
-		goto_bad;
+	if (vg_read_error(vg)) {
+		if (vg_read_error(vg) == FAILED_EXIST)
+			log_error("A volume group called %s already exists.", vp_new.vg_name);
+		else
+			log_error("Can't get lock for %s", vp_new.vg_name);
+		goto bad;
+	}
 
 	if (!vg_set_extent_size(vg, vp_new.extent_size) ||
 	    !vg_set_max_lv(vg, vp_new.max_lv) ||
-- 
1.6.5.3

[PATCH] Print error if VG already exist.

[Dave Wysochanski]

On Thu, 2009-12-03 at 15:31 +0100, Milan Broz wrote:
> This test have to be moved because of new vg read error handling.
> 
> Signed-off-by: Milan Broz <mbroz@redhat.com>
> ---
>  tools/vgcreate.c |    9 +++++++--
>  1 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/vgcreate.c b/tools/vgcreate.c
> index b1b0c63..8aef7a2 100644
> --- a/tools/vgcreate.c
> +++ b/tools/vgcreate.c
> @@ -55,8 +55,13 @@ int vgcreate(struct cmd_context *cmd, int argc, char **argv)
>  
>  	/* Create the new VG */
>  	vg = vg_create(cmd, vp_new.vg_name);
> -	if (vg_read_error(vg))
> -		goto_bad;
> +	if (vg_read_error(vg)) {
> +		if (vg_read_error(vg) == FAILED_EXIST)
> +			log_error("A volume group called %s already exists.", vp_new.vg_name);
> +		else
> +			log_error("Can't get lock for %s", vp_new.vg_name);
> +		goto bad;
> +	}
>  
>  	if (!vg_set_extent_size(vg, vp_new.extent_size) ||
>  	    !vg_set_max_lv(vg, vp_new.max_lv) ||

Ack.

Some more cleanup probably warranted but another time and there are
already FIXMEs in the code.

[PATCH] Print error if VG already exist.

[Petr Rockai]

Milan Broz <mbroz@redhat.com> writes:
> This test have to be moved because of new vg read error handling.

> @@ -55,8 +55,13 @@ int vgcreate(struct cmd_context *cmd, int argc, char **argv)
>  
>  	/* Create the new VG */
>  	vg = vg_create(cmd, vp_new.vg_name);
> -	if (vg_read_error(vg))
> -		goto_bad;
> +	if (vg_read_error(vg)) {
> +		if (vg_read_error(vg) == FAILED_EXIST)
> +			log_error("A volume group called %s already exists.", vp_new.vg_name);
> +		else
> +			log_error("Can't get lock for %s", vp_new.vg_name);
> +		goto bad;
> +	}

This is suspicious. In vg_create:

	if ((vg = vg_read_internal(cmd, vg_name, NULL, &consistent))) {
		log_error("A volume group called '%s' already exists.", vg_name);
		unlock_and_release_vg(cmd, vg, vg_name);
		return _vg_make_handle(cmd, NULL, FAILED_EXIST);
	}

So the code you are adding should not be needed, at least not for the
EXIST case.

Hmm. Ok, I see that the code is actually broken, since what fails is
already vg_lock_newname. I'd argue that it should be vg_lock_newname
that should be printing this, not vgcreate. Like this:

diff -u -p -r1.299 metadata.c
--- lib/metadata/metadata.c	24 Nov 2009 22:55:56 -0000	1.299
+++ lib/metadata/metadata.c	3 Dec 2009 20:11:56 -0000
@@ -3507,6 +3507,7 @@ uint32_t vg_lock_newname(struct cmd_cont
 
 	/* Found vgname so cannot reserve. */
 	unlock_vg(cmd, vgname);
+	log_error("A volume group called '%s' already exists.", vg_name);
 	return FAILED_EXIST;
 }

Yours,
   Petr.

[PATCH] Print error if VG already exist.

[Dave Wysochanski]

On Thu, 2009-12-03 at 21:13 +0100, Petr Rockai wrote:
> Milan Broz <mbroz@redhat.com> writes:
> > This test have to be moved because of new vg read error handling.
> 
> > @@ -55,8 +55,13 @@ int vgcreate(struct cmd_context *cmd, int argc, char **argv)
> >  
> >  	/* Create the new VG */
> >  	vg = vg_create(cmd, vp_new.vg_name);
> > -	if (vg_read_error(vg))
> > -		goto_bad;
> > +	if (vg_read_error(vg)) {
> > +		if (vg_read_error(vg) == FAILED_EXIST)
> > +			log_error("A volume group called %s already exists.", vp_new.vg_name);
> > +		else
> > +			log_error("Can't get lock for %s", vp_new.vg_name);
> > +		goto bad;
> > +	}
> 
> This is suspicious. In vg_create:
> 
> 	if ((vg = vg_read_internal(cmd, vg_name, NULL, &consistent))) {
> 		log_error("A volume group called '%s' already exists.", vg_name);
> 		unlock_and_release_vg(cmd, vg, vg_name);
> 		return _vg_make_handle(cmd, NULL, FAILED_EXIST);
> 	}
> 
> So the code you are adding should not be needed, at least not for the
> EXIST case.
> 
> Hmm. Ok, I see that the code is actually broken, since what fails is
> already vg_lock_newname. I'd argue that it should be vg_lock_newname
> that should be printing this, not vgcreate. Like this:
> 
> diff -u -p -r1.299 metadata.c
> --- lib/metadata/metadata.c	24 Nov 2009 22:55:56 -0000	1.299
> +++ lib/metadata/metadata.c	3 Dec 2009 20:11:56 -0000
> @@ -3507,6 +3507,7 @@ uint32_t vg_lock_newname(struct cmd_cont
>  
>  	/* Found vgname so cannot reserve. */
>  	unlock_vg(cmd, vgname);
> +	log_error("A volume group called '%s' already exists.", vg_name);
>  	return FAILED_EXIST;
>  }
> 

Unfortunately this will add extraneous messages / errors to some vgsplit
operations as a check for existence is not always a command error.  Plus
vg_lock_newname only returns codes in the other paths and it would be
good to stay consistent (log_error() in all error paths or none of them
and let caller decide).