[Libvir] [PATCH] check the file name for --log
Hi, This patch fixes the trouble if execute virsh with virsh --log --debug. In this case FILE "--debug" is generated and --debug is off. This patch treat as error return if it uses the hyphen "-" at the head of log file name. Signed-off-by: Atsushi SAKAI <sakaia@jp.fujitsu.com> Thanks Atsushi SAKAI
Atsushi SAKAI wrote:
Hi,
This patch fixes the trouble if execute virsh with virsh --log --debug. In this case FILE "--debug" is generated and --debug is off.
This patch treat as error return if it uses the hyphen "-" at the head of log file name.
+1 with reservations. I'm a bit surprised that getopt_long doesn't do the right thing here. With your patch we won't be able to have logfiles which begin with a '-' character. I'm not sure whether that's going to be a problem. Rich. -- Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/ Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 03798903
On Tue, Jul 31, 2007 at 08:12:56PM +0900, Atsushi SAKAI wrote:
Hi,
This patch fixes the trouble if execute virsh with virsh --log --debug. In this case FILE "--debug" is generated and --debug is off.
Well it does what the user asked, is that really an error ? Trying to guess what the user meant instead of executing what it asked for is a good way to break other things. I'm not sure this should be done that way.
This patch treat as error return if it uses the hyphen "-" at the head of log file name.
If you really want to catch this then I will ask you to also update the documentation for that option to explain that using a filename starting with - is not accepted. Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
On Tue, Jul 31, 2007 at 07:38:18AM -0400, Daniel Veillard wrote:
On Tue, Jul 31, 2007 at 08:12:56PM +0900, Atsushi SAKAI wrote:
Hi,
This patch fixes the trouble if execute virsh with virsh --log --debug. In this case FILE "--debug" is generated and --debug is off.
Well it does what the user asked, is that really an error ? Trying to guess what the user meant instead of executing what it asked for is a good way to break other things. I'm not sure this should be done that way.
This patch treat as error return if it uses the hyphen "-" at the head of log file name.
If you really want to catch this then I will ask you to also update the documentation for that option to explain that using a filename starting with - is not accepted.
Trying to catch this is a can of worms. You want to put similar 'validation' into every other command line option ? This is special casing --log and ignoring the same problem in every other command line option we have. It is a waste of time & uneccessary complication. Every other UNIX command using getopt has the same behaviour. Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
On Tue, Jul 31, 2007 at 08:12:56PM +0900, Atsushi SAKAI wrote:
Hi,
This patch fixes the trouble if execute virsh with virsh --log --debug. In this case FILE "--debug" is generated and --debug is off.
This patch treat as error return if it uses the hyphen "-" at the head of log file name.
NACK. The code is already doing exactly what the user asked. The --log flag takes an argument and you are supplied --debug as that argument. Therefore it creates the file --debug. This is fine. Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
Hi, Dan Then This kind of Warning is acceptable? Thanks Atsushi SAKAI "Daniel P. Berrange" <berrange@redhat.com> wrote:
On Tue, Jul 31, 2007 at 08:12:56PM +0900, Atsushi SAKAI wrote:
Hi,
This patch fixes the trouble if execute virsh with virsh --log --debug. In this case FILE "--debug" is generated and --debug is off.
This patch treat as error return if it uses the hyphen "-" at the head of log file name.
NACK. The code is already doing exactly what the user asked. The --log flag takes an argument and you are supplied --debug as that argument. Therefore it creates the file --debug. This is fine.
Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
Atsushi SAKAI wrote:
Hi, Dan
Then This kind of Warning is acceptable?
I don't have a problem with this sort of warning. Anyone else...? Rich. -- Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/ Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 03798903
On Mon, Aug 20, 2007 at 01:13:54PM +0100, Richard W.M. Jones wrote:
Atsushi SAKAI wrote:
Hi, Dan
Then This kind of Warning is acceptable?
I don't have a problem with this sort of warning. Anyone else...?
Hum, why not, though if we start adding warnings, then I would add a vshWarning function, inspired from vshError, and allow a --nowarning flag to avoid them. It's okay to be a bit pedantic, as long as the user has a way around if he knows what he's doing. Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
On Mon, Aug 20, 2007 at 08:21:53AM -0400, Daniel Veillard wrote:
On Mon, Aug 20, 2007 at 01:13:54PM +0100, Richard W.M. Jones wrote:
Atsushi SAKAI wrote:
Hi, Dan
Then This kind of Warning is acceptable?
I don't have a problem with this sort of warning. Anyone else...?
Hum, why not, though if we start adding warnings, then I would add a vshWarning function, inspired from vshError, and allow a --nowarning flag to avoid them. It's okay to be a bit pedantic, as long as the user has a way around if he knows what he's doing.
Personally I'm against adding a warning for the same reasons as I'm against making it a hard error. The existing behaviour is standard behaviour of all UNIX command line tools. If the flag requires an argument no other tool tries to second guess whether the value supplied is bogus or not, because it will fundamentally always be a *guess*. Erroring, or printing warnings about things you're guessing might be wrong is just an annoyance Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
participants (4)
-
Atsushi SAKAI -
Daniel P. Berrange -
Daniel Veillard -
Richard W.M. Jones