* [PATCH] Print error if VG already exist. @ 2009-12-03 14:31 Milan Broz 2009-12-03 17:15 ` Dave Wysochanski 2009-12-03 20:13 ` Petr Rockai 0 siblings, 2 replies; 4+ messages in thread From: Milan Broz @ 2009-12-03 14:31 UTC (permalink / raw) To: lvm-devel 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH] Print error if VG already exist. 2009-12-03 14:31 [PATCH] Print error if VG already exist Milan Broz @ 2009-12-03 17:15 ` Dave Wysochanski 2009-12-03 20:13 ` Petr Rockai 1 sibling, 0 replies; 4+ messages in thread From: Dave Wysochanski @ 2009-12-03 17:15 UTC (permalink / raw) To: lvm-devel 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. ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH] Print error if VG already exist. 2009-12-03 14:31 [PATCH] Print error if VG already exist Milan Broz 2009-12-03 17:15 ` Dave Wysochanski @ 2009-12-03 20:13 ` Petr Rockai 2009-12-03 20:59 ` Dave Wysochanski 1 sibling, 1 reply; 4+ messages in thread From: Petr Rockai @ 2009-12-03 20:13 UTC (permalink / raw) To: lvm-devel 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. ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH] Print error if VG already exist. 2009-12-03 20:13 ` Petr Rockai @ 2009-12-03 20:59 ` Dave Wysochanski 0 siblings, 0 replies; 4+ messages in thread From: Dave Wysochanski @ 2009-12-03 20:59 UTC (permalink / raw) To: lvm-devel 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). ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2009-12-03 20:59 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-12-03 14:31 [PATCH] Print error if VG already exist Milan Broz 2009-12-03 17:15 ` Dave Wysochanski 2009-12-03 20:13 ` Petr Rockai 2009-12-03 20:59 ` Dave Wysochanski
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.