[libvirt] [libvirt-sandbox][PATCH 0/2] Fix some issues in virt-sandbox-service
Alex Jia (2): Fix logical judgement in get_name Raise clear error message if no legacy configuration bin/virt-sandbox-service | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) -- 1.8.3.1
Signed-off-by: Alex Jia <ajia@redhat.com> --- bin/virt-sandbox-service | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/bin/virt-sandbox-service b/bin/virt-sandbox-service index 03873c9..26b4a40 100755 --- a/bin/virt-sandbox-service +++ b/bin/virt-sandbox-service @@ -453,8 +453,12 @@ WantedBy=multi-user.target def get_name(self): if self.config: - return self.config.get_name() - raise ValueError([_("Name not configured")]) + name = self.config.get_name() + if not name: + raise ValueError([_("Name not configured")]) + return name + sys.stderr.write("The configuration %s does not exist\n" % self.config) + sys.exit(1) def set_copy(self, copy): self.copy = copy -- 1.8.3.1
On Fri, Aug 09, 2013 at 06:26:46PM +0800, Alex Jia wrote: Please explain the scenario where you hit the flaw in the commit message. I can see what you've changed, but I don't see why you have changed it. The commit message must describe the 'why'.
Signed-off-by: Alex Jia <ajia@redhat.com> --- bin/virt-sandbox-service | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/bin/virt-sandbox-service b/bin/virt-sandbox-service index 03873c9..26b4a40 100755 --- a/bin/virt-sandbox-service +++ b/bin/virt-sandbox-service @@ -453,8 +453,12 @@ WantedBy=multi-user.target
def get_name(self): if self.config: - return self.config.get_name() - raise ValueError([_("Name not configured")]) + name = self.config.get_name() + if not name: + raise ValueError([_("Name not configured")]) + return name + sys.stderr.write("The configuration %s does not exist\n" % self.config) + sys.exit(1)
def set_copy(self, copy): self.copy = copy
Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 08/09/2013 06:29 PM, Daniel P. Berrange wrote:
On Fri, Aug 09, 2013 at 06:26:46PM +0800, Alex Jia wrote:
Please explain the scenario where you hit the flaw in the commit message. I can see what you've changed, but I don't see why you have changed it. The commit message must describe the 'why'.
ok, I will explain it in v2.
Signed-off-by: Alex Jia<ajia@redhat.com> --- bin/virt-sandbox-service | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/bin/virt-sandbox-service b/bin/virt-sandbox-service index 03873c9..26b4a40 100755 --- a/bin/virt-sandbox-service +++ b/bin/virt-sandbox-service @@ -453,8 +453,12 @@ WantedBy=multi-user.target
def get_name(self): if self.config: - return self.config.get_name() - raise ValueError([_("Name not configured")]) + name = self.config.get_name() + if not name: + raise ValueError([_("Name not configured")]) + return name + sys.stderr.write("The configuration %s does not exist\n" % self.config) + sys.exit(1)
def set_copy(self, copy): self.copy = copy Daniel
Signed-off-by: Alex Jia <ajia@redhat.com> --- bin/virt-sandbox-service | 3 +++ 1 file changed, 3 insertions(+) diff --git a/bin/virt-sandbox-service b/bin/virt-sandbox-service index 26b4a40..cb40f6a 100755 --- a/bin/virt-sandbox-service +++ b/bin/virt-sandbox-service @@ -965,6 +965,9 @@ def upgrade_config(args): configfile = get_legacy_config_path(args.name) if os.path.exists(configfile): upgrade_config_legacy(configfile) + else: + sys.stderr.write("No legacy '%s' configuration\n" % args.name) + sys.exit(1) def upgrade_filesystem(args): -- 1.8.3.1
On Fri, Aug 09, 2013 at 06:26:47PM +0800, Alex Jia wrote:
Signed-off-by: Alex Jia <ajia@redhat.com> --- bin/virt-sandbox-service | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/bin/virt-sandbox-service b/bin/virt-sandbox-service index 26b4a40..cb40f6a 100755 --- a/bin/virt-sandbox-service +++ b/bin/virt-sandbox-service @@ -965,6 +965,9 @@ def upgrade_config(args): configfile = get_legacy_config_path(args.name) if os.path.exists(configfile): upgrade_config_legacy(configfile) + else: + sys.stderr.write("No legacy '%s' configuration\n" % args.name) + sys.exit(1)
This isn't desired. This command is intended to be a no-op if nothing needs changing. It is not just about upgrading from this legacy config file layout. In the future I expet us to add more code here as we make further changes. So it is right to silently exit with success here, not report an error. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 08/09/2013 06:30 PM, Daniel P. Berrange wrote:
On Fri, Aug 09, 2013 at 06:26:47PM +0800, Alex Jia wrote:
Signed-off-by: Alex Jia<ajia@redhat.com> --- bin/virt-sandbox-service | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/bin/virt-sandbox-service b/bin/virt-sandbox-service index 26b4a40..cb40f6a 100755 --- a/bin/virt-sandbox-service +++ b/bin/virt-sandbox-service @@ -965,6 +965,9 @@ def upgrade_config(args): configfile = get_legacy_config_path(args.name) if os.path.exists(configfile): upgrade_config_legacy(configfile) + else: + sys.stderr.write("No legacy '%s' configuration\n" % args.name) + sys.exit(1) This isn't desired. This command is intended to be a no-op if nothing needs changing. It is not just about upgrading from this legacy config file layout. In the future I expet us to add more code here as we make further changes. So it is right to silently exit with success here, not report an error.
Ok, got it and thanks for your review.
Daniel
participants (2)
-
Alex Jia -
Daniel P. Berrange