| Message ID | 20210330215039.3024315-1-blp@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] ovsdb2ddlog2c: Fix behavior for internal error. | expand |
On 3/30/21 11:50 PM, Ben Pfaff wrote: > Without this change, ovsdb2ddlog2c exited successfully if it ran into an > option that it was supposed to understand but didn't implement (which > is a bug). This commit makes it raise an exception instead. > > Signed-off-by: Ben Pfaff <blp@ovn.org> > Reported-by: Dumitru Ceara <dceara@redhat.com> > --- > northd/ovsdb2ddlog2c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/northd/ovsdb2ddlog2c b/northd/ovsdb2ddlog2c > index 19aeb265b633..6d5ee9b4b45d 100755 > --- a/northd/ovsdb2ddlog2c > +++ b/northd/ovsdb2ddlog2c > @@ -82,7 +82,7 @@ if __name__ == "__main__": > elif key == '--output-file': > output_file = value > else: > - sys.exit(0) > + assert False I agree with the assert here because it's a bug when valid arguments cannot be handled. However, I wonder if it's worth changing it to something like: assert False, "%s: Cannot handle argument: %s" % (argv0, key) In any case, the fix works: Acked-by: Dumitru Ceara <dceara@redhat.com> > > if schema_file is None: > sys.stderr.write("%s: missing -f or --schema-file option\n" % argv0) >
On Wed, Mar 31, 2021 at 09:35:36AM +0200, Dumitru Ceara wrote: > On 3/30/21 11:50 PM, Ben Pfaff wrote: > > Without this change, ovsdb2ddlog2c exited successfully if it ran into an > > option that it was supposed to understand but didn't implement (which > > is a bug). This commit makes it raise an exception instead. > > > > Signed-off-by: Ben Pfaff <blp@ovn.org> > > Reported-by: Dumitru Ceara <dceara@redhat.com> > > --- > > northd/ovsdb2ddlog2c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/northd/ovsdb2ddlog2c b/northd/ovsdb2ddlog2c > > index 19aeb265b633..6d5ee9b4b45d 100755 > > --- a/northd/ovsdb2ddlog2c > > +++ b/northd/ovsdb2ddlog2c > > @@ -82,7 +82,7 @@ if __name__ == "__main__": > > elif key == '--output-file': > > output_file = value > > else: > > - sys.exit(0) > > + assert False > > I agree with the assert here because it's a bug when valid arguments > cannot be handled. However, I wonder if it's worth changing it to > something like: > > assert False, "%s: Cannot handle argument: %s" % (argv0, key) > > In any case, the fix works: > > Acked-by: Dumitru Ceara <dceara@redhat.com> I think the bug will be equally obvious to the developer who has to look at it, either way, so I went with the simpler version. Thanks, I applied this to master.
diff --git a/northd/ovsdb2ddlog2c b/northd/ovsdb2ddlog2c index 19aeb265b633..6d5ee9b4b45d 100755 --- a/northd/ovsdb2ddlog2c +++ b/northd/ovsdb2ddlog2c @@ -82,7 +82,7 @@ if __name__ == "__main__": elif key == '--output-file': output_file = value else: - sys.exit(0) + assert False if schema_file is None: sys.stderr.write("%s: missing -f or --schema-file option\n" % argv0)
Without this change, ovsdb2ddlog2c exited successfully if it ran into an option that it was supposed to understand but didn't implement (which is a bug). This commit makes it raise an exception instead. Signed-off-by: Ben Pfaff <blp@ovn.org> Reported-by: Dumitru Ceara <dceara@redhat.com> --- northd/ovsdb2ddlog2c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)