On Wed, Mar 15, 2017 at 10:36:35AM +0000, Dr. David Alan Gilbert wrote: > * Markus Armbruster (arm...@redhat.com) wrote: > > "Dr. David Alan Gilbert" <dgilb...@redhat.com> writes: > > > > > * Markus Armbruster (arm...@redhat.com) wrote: > > >> "Dr. David Alan Gilbert" <dgilb...@redhat.com> writes: > > [...] > > >> > I'm confused why we need a 'reset to default' - all we need is the > > >> > ability > > >> > to change each parameter, and for the new value of that parameter > > >> > to be an empty string. > > >> > > >> You argue syntax, I'm arguing semantics. > > >> > > >> The command means "set parameter P to value V". *Except* when V is "", > > >> it means something else, namely "reset parameter P to its default, > > >> whatever that may be". > > >> > > >> This is (a) not general, because it won't do for cases where "" may > > >> occur as value, and (b) ugly. > > >> > > >> Ugliness is the eye of the beholder. Lack of generality isn't. > > > > > > No, I'm questioning why it's defined as 'reset parameter P to its > > > default'; > > > why do we need a way to do that? > > > > Dan's commit message explains: > > > > Some of the migration parameters are strings, which default to NULL, > > eg tls_hostname and tls_creds. > > > > The mgmt app will set the tls_creds parameter on both source and target > > QEMU instances, in order to trigger use of TLS for migration. > > > > After performing a TLS encrypted migration though, migration might be > > used for other reasons - for example, to save the QEMU state to a file. > > We need TLS turned off when doing this, but the migrate-set-parameters > > QAPI command does not provide any facility to clear/reset parameters > > to their default state. > > > > If you simply ommit the tls_creds parameter in migrate-set-parameters, > > then 'has_tls_creds' will be false and so no action will be taken. The > > only option that works with migrate-set-parameters is to treat "" on > > the wire as equivalent to requesting NULL. Failing that we would have > > to create a new 'migrate-reset-parameters' method to explicitly put > > a parameter back to its default value. > > OK, I thought the default was empty string. > IMHO the problem here is we're just being too clever; lets just make > the default "" rather than NULL, and make that parameter always be > a string.
>From an internal migration code POV I think NULL is right, but none the less I'll create a patch showing what using "" internally would look like so we can compare Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
