Re: [Qemu-devel] [PATCH] migration: allow clearing migration string parameters

2017-03-15 Thread Daniel P. Berrange 
On Wed, Mar 15, 2017 at 10:36:35AM +, Dr. David Alan Gilbert wrote:
> * Markus Armbruster (arm...@redhat.com) wrote:
> > "Dr. David Alan Gilbert"  writes:
> > 
> > > * Markus Armbruster (arm...@redhat.com) wrote:
> > >> "Dr. David Alan Gilbert"  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/ :|

Re: [Qemu-devel] [PATCH] migration: allow clearing migration string parameters

2017-03-15 Thread Dr. David Alan Gilbert 
* Markus Armbruster (arm...@redhat.com) wrote:
> "Dr. David Alan Gilbert"  writes:
> 
> > * Markus Armbruster (arm...@redhat.com) wrote:
> >> "Dr. David Alan Gilbert"  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.

Dave

--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH] migration: allow clearing migration string parameters

2017-03-15 Thread Markus Armbruster 
"Dr. David Alan Gilbert"  writes:

> * Markus Armbruster (arm...@redhat.com) wrote:
>> "Dr. David Alan Gilbert"  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.

Re: [Qemu-devel] [PATCH] migration: allow clearing migration string parameters

2017-03-15 Thread Dr. David Alan Gilbert 
* Markus Armbruster (arm...@redhat.com) wrote:
> "Dr. David Alan Gilbert"  writes:
> 
> > * Markus Armbruster (arm...@redhat.com) wrote:
> >> "Daniel P. Berrange"  writes:
> >> 
> >> > On Wed, Mar 01, 2017 at 08:36:03AM -0600, Eric Blake wrote:
> >> >> On 03/01/2017 06:32 AM, Daniel P. Berrange wrote:
> >> >> >  }
> >> >> >  if (params->has_tls_creds) {
> >> >> >  g_free(s->parameters.tls_creds);
> >> >> > -s->parameters.tls_creds = g_strdup(params->tls_creds);
> >> >> > +if (*params->tls_creds == '\0') {
> >> >> > +s->parameters.tls_creds = NULL;
> >> >> 
> >> >> I'm wondering if you should also do s->parameters.has_tls_creds = false
> >> >> at this point?  The visitors expect that if has_tls_creds is true, then
> >> >> the string is non-NULL.
> >> >
> >> > The fact that s->parameters contains has_* fields is completely ignored
> >> > by the migration code afaict. IOW the code behaves as if all the has_*
> >> > fields are hardwired to true in s->parameters, even though that is not
> >> > the case :-) The has_* fields are only used when the various migration
> >> > QMP methods are executed, and those all use a separate 
> >> > MigrationParameters
> >> > struct instance.
> >> 
> >> Not keeping the has_ members up-to-date is harmless as long as you don't
> >> pass the thing to visitors, including the one hiding in qapi_free_FOO().
> >> That one ignores scalars, though.
> >> 
> >> 
> >> From a more abstract point of view, we have two related data types: one
> >> for the state, and one for state changes requests.
> >> 
> >> In state, members are always present.
> >> 
> >> A state change request is a bag of state member change requests, and
> >> each request can either specify the new value or ask for a reset to
> >> default.  Absent member means no change.
> >> 
> >> We press the same QAPI type into service for both by making all members
> >> optional.
> >> 
> >> For the state case, we hardwire the has_ to true.  Or even ignore them
> >> completely.
> >> 
> >> For the state change request, we use has_ = false for "no change", has_
> >> = true with a special value for "reset to default" (new in this patch)
> >> and has_ = true with a non-special value for "set to this value".
> >
> > 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?

Dave

> >> Requires a special value outside the set of non-special values.  The
> >> obvious one is JSON null, but the QAPI generator doesn't quite support
> >> that, yet.  "" works here, but is not general.
> >> 
> >> I think I can get you null support in 2.10.  Would that work for you?
> >
> > Dave
> > --
> > Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH] migration: allow clearing migration string parameters

2017-03-15 Thread Markus Armbruster 
"Dr. David Alan Gilbert"  writes:

> * Markus Armbruster (arm...@redhat.com) wrote:
>> "Daniel P. Berrange"  writes:
>> 
>> > On Wed, Mar 01, 2017 at 08:36:03AM -0600, Eric Blake wrote:
>> >> On 03/01/2017 06:32 AM, Daniel P. Berrange wrote:
>> >> >  }
>> >> >  if (params->has_tls_creds) {
>> >> >  g_free(s->parameters.tls_creds);
>> >> > -s->parameters.tls_creds = g_strdup(params->tls_creds);
>> >> > +if (*params->tls_creds == '\0') {
>> >> > +s->parameters.tls_creds = NULL;
>> >> 
>> >> I'm wondering if you should also do s->parameters.has_tls_creds = false
>> >> at this point?  The visitors expect that if has_tls_creds is true, then
>> >> the string is non-NULL.
>> >
>> > The fact that s->parameters contains has_* fields is completely ignored
>> > by the migration code afaict. IOW the code behaves as if all the has_*
>> > fields are hardwired to true in s->parameters, even though that is not
>> > the case :-) The has_* fields are only used when the various migration
>> > QMP methods are executed, and those all use a separate MigrationParameters
>> > struct instance.
>> 
>> Not keeping the has_ members up-to-date is harmless as long as you don't
>> pass the thing to visitors, including the one hiding in qapi_free_FOO().
>> That one ignores scalars, though.
>> 
>> 
>> From a more abstract point of view, we have two related data types: one
>> for the state, and one for state changes requests.
>> 
>> In state, members are always present.
>> 
>> A state change request is a bag of state member change requests, and
>> each request can either specify the new value or ask for a reset to
>> default.  Absent member means no change.
>> 
>> We press the same QAPI type into service for both by making all members
>> optional.
>> 
>> For the state case, we hardwire the has_ to true.  Or even ignore them
>> completely.
>> 
>> For the state change request, we use has_ = false for "no change", has_
>> = true with a special value for "reset to default" (new in this patch)
>> and has_ = true with a non-special value for "set to this value".
>
> 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.

>> Requires a special value outside the set of non-special values.  The
>> obvious one is JSON null, but the QAPI generator doesn't quite support
>> that, yet.  "" works here, but is not general.
>> 
>> I think I can get you null support in 2.10.  Would that work for you?
>
> Dave
> --
> Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH] migration: allow clearing migration string parameters

2017-03-14 Thread Dr. David Alan Gilbert 
* Markus Armbruster (arm...@redhat.com) wrote:
> "Daniel P. Berrange"  writes:
> 
> > On Wed, Mar 01, 2017 at 08:36:03AM -0600, Eric Blake wrote:
> >> On 03/01/2017 06:32 AM, Daniel P. Berrange wrote:
> >> >  }
> >> >  if (params->has_tls_creds) {
> >> >  g_free(s->parameters.tls_creds);
> >> > -s->parameters.tls_creds = g_strdup(params->tls_creds);
> >> > +if (*params->tls_creds == '\0') {
> >> > +s->parameters.tls_creds = NULL;
> >> 
> >> I'm wondering if you should also do s->parameters.has_tls_creds = false
> >> at this point?  The visitors expect that if has_tls_creds is true, then
> >> the string is non-NULL.
> >
> > The fact that s->parameters contains has_* fields is completely ignored
> > by the migration code afaict. IOW the code behaves as if all the has_*
> > fields are hardwired to true in s->parameters, even though that is not
> > the case :-) The has_* fields are only used when the various migration
> > QMP methods are executed, and those all use a separate MigrationParameters
> > struct instance.
> 
> Not keeping the has_ members up-to-date is harmless as long as you don't
> pass the thing to visitors, including the one hiding in qapi_free_FOO().
> That one ignores scalars, though.
> 
> 
> From a more abstract point of view, we have two related data types: one
> for the state, and one for state changes requests.
> 
> In state, members are always present.
> 
> A state change request is a bag of state member change requests, and
> each request can either specify the new value or ask for a reset to
> default.  Absent member means no change.
> 
> We press the same QAPI type into service for both by making all members
> optional.
> 
> For the state case, we hardwire the has_ to true.  Or even ignore them
> completely.
> 
> For the state change request, we use has_ = false for "no change", has_
> = true with a special value for "reset to default" (new in this patch)
> and has_ = true with a non-special value for "set to this value".

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.

> Requires a special value outside the set of non-special values.  The
> obvious one is JSON null, but the QAPI generator doesn't quite support
> that, yet.  "" works here, but is not general.
> 
> I think I can get you null support in 2.10.  Would that work for you?

Dave
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH] migration: allow clearing migration string parameters

2017-03-03 Thread Daniel P. Berrange 
On Fri, Mar 03, 2017 at 06:05:15PM +0100, Markus Armbruster wrote:
> "Daniel P. Berrange"  writes:
> 
> > On Fri, Mar 03, 2017 at 03:44:12PM +0100, Markus Armbruster wrote:
> >> "Daniel P. Berrange"  writes:
> >> 
> >> > On Wed, Mar 01, 2017 at 08:36:03AM -0600, Eric Blake wrote:
> >> >> On 03/01/2017 06:32 AM, Daniel P. Berrange wrote:
> >> >> >  }
> >> >> >  if (params->has_tls_creds) {
> >> >> >  g_free(s->parameters.tls_creds);
> >> >> > -s->parameters.tls_creds = g_strdup(params->tls_creds);
> >> >> > +if (*params->tls_creds == '\0') {
> >> >> > +s->parameters.tls_creds = NULL;
> >> >> 
> >> >> I'm wondering if you should also do s->parameters.has_tls_creds = false
> >> >> at this point?  The visitors expect that if has_tls_creds is true, then
> >> >> the string is non-NULL.
> >> >
> >> > The fact that s->parameters contains has_* fields is completely ignored
> >> > by the migration code afaict. IOW the code behaves as if all the has_*
> >> > fields are hardwired to true in s->parameters, even though that is not
> >> > the case :-) The has_* fields are only used when the various migration
> >> > QMP methods are executed, and those all use a separate 
> >> > MigrationParameters
> >> > struct instance.
> >> 
> >> Not keeping the has_ members up-to-date is harmless as long as you don't
> >> pass the thing to visitors, including the one hiding in qapi_free_FOO().
> >> That one ignores scalars, though.
> >> 
> >> 
> >> From a more abstract point of view, we have two related data types: one
> >> for the state, and one for state changes requests.
> >> 
> >> In state, members are always present.
> >> 
> >> A state change request is a bag of state member change requests, and
> >> each request can either specify the new value or ask for a reset to
> >> default.  Absent member means no change.
> >> 
> >> We press the same QAPI type into service for both by making all members
> >> optional.
> >> 
> >> For the state case, we hardwire the has_ to true.  Or even ignore them
> >> completely.
> >> 
> >> For the state change request, we use has_ = false for "no change", has_
> >> = true with a special value for "reset to default" (new in this patch)
> >> and has_ = true with a non-special value for "set to this value".
> >> 
> >> Requires a special value outside the set of non-special values.  The
> >> obvious one is JSON null, but the QAPI generator doesn't quite support
> >> that, yet.  "" works here, but is not general.
> >> 
> >> I think I can get you null support in 2.10.  Would that work for you?
> >
> > Not really - we need this fix so libvirt can enable TLS migration,
> > in addition we'll be wanting to add it to stable branches upstream
> > and downstream.
> 
> Alternatives:
> 
> (a) We support null in 2.9, and use it to solve this problem.
> 
> (b) We use "", support null in 2.10, immediately deprecate "" in favor
> of null.
> 
> PRO (a): Less churn, less variation, less overall work.
>
> PRO (b): Might be easier to backport to really old versions.  Soft
> freeze.
> 
> Non-alternative: use "" forever.  Sorry, I hate it.

I don't see a problem with accepting both "" & nil forever, as there's
no reason to need to distinguish them here. "" is never a valid ID for
a qobject, and "" is never a valid hostname, so treating "" as a synonym
for NULL/nil is fine.

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/ :|

Re: [Qemu-devel] [PATCH] migration: allow clearing migration string parameters

2017-03-03 Thread Markus Armbruster 
"Daniel P. Berrange"  writes:

> On Fri, Mar 03, 2017 at 03:44:12PM +0100, Markus Armbruster wrote:
>> "Daniel P. Berrange"  writes:
>> 
>> > On Wed, Mar 01, 2017 at 08:36:03AM -0600, Eric Blake wrote:
>> >> On 03/01/2017 06:32 AM, Daniel P. Berrange wrote:
>> >> >  }
>> >> >  if (params->has_tls_creds) {
>> >> >  g_free(s->parameters.tls_creds);
>> >> > -s->parameters.tls_creds = g_strdup(params->tls_creds);
>> >> > +if (*params->tls_creds == '\0') {
>> >> > +s->parameters.tls_creds = NULL;
>> >> 
>> >> I'm wondering if you should also do s->parameters.has_tls_creds = false
>> >> at this point?  The visitors expect that if has_tls_creds is true, then
>> >> the string is non-NULL.
>> >
>> > The fact that s->parameters contains has_* fields is completely ignored
>> > by the migration code afaict. IOW the code behaves as if all the has_*
>> > fields are hardwired to true in s->parameters, even though that is not
>> > the case :-) The has_* fields are only used when the various migration
>> > QMP methods are executed, and those all use a separate MigrationParameters
>> > struct instance.
>> 
>> Not keeping the has_ members up-to-date is harmless as long as you don't
>> pass the thing to visitors, including the one hiding in qapi_free_FOO().
>> That one ignores scalars, though.
>> 
>> 
>> From a more abstract point of view, we have two related data types: one
>> for the state, and one for state changes requests.
>> 
>> In state, members are always present.
>> 
>> A state change request is a bag of state member change requests, and
>> each request can either specify the new value or ask for a reset to
>> default.  Absent member means no change.
>> 
>> We press the same QAPI type into service for both by making all members
>> optional.
>> 
>> For the state case, we hardwire the has_ to true.  Or even ignore them
>> completely.
>> 
>> For the state change request, we use has_ = false for "no change", has_
>> = true with a special value for "reset to default" (new in this patch)
>> and has_ = true with a non-special value for "set to this value".
>> 
>> Requires a special value outside the set of non-special values.  The
>> obvious one is JSON null, but the QAPI generator doesn't quite support
>> that, yet.  "" works here, but is not general.
>> 
>> I think I can get you null support in 2.10.  Would that work for you?
>
> Not really - we need this fix so libvirt can enable TLS migration,
> in addition we'll be wanting to add it to stable branches upstream
> and downstream.

Alternatives:

(a) We support null in 2.9, and use it to solve this problem.

(b) We use "", support null in 2.10, immediately deprecate "" in favor
of null.

PRO (a): Less churn, less variation, less overall work.

PRO (b): Might be easier to backport to really old versions.  Soft
freeze.

Non-alternative: use "" forever.  Sorry, I hate it.

Re: [Qemu-devel] [PATCH] migration: allow clearing migration string parameters

2017-03-03 Thread Daniel P. Berrange 
On Fri, Mar 03, 2017 at 03:44:12PM +0100, Markus Armbruster wrote:
> "Daniel P. Berrange"  writes:
> 
> > On Wed, Mar 01, 2017 at 08:36:03AM -0600, Eric Blake wrote:
> >> On 03/01/2017 06:32 AM, Daniel P. Berrange wrote:
> >> >  }
> >> >  if (params->has_tls_creds) {
> >> >  g_free(s->parameters.tls_creds);
> >> > -s->parameters.tls_creds = g_strdup(params->tls_creds);
> >> > +if (*params->tls_creds == '\0') {
> >> > +s->parameters.tls_creds = NULL;
> >> 
> >> I'm wondering if you should also do s->parameters.has_tls_creds = false
> >> at this point?  The visitors expect that if has_tls_creds is true, then
> >> the string is non-NULL.
> >
> > The fact that s->parameters contains has_* fields is completely ignored
> > by the migration code afaict. IOW the code behaves as if all the has_*
> > fields are hardwired to true in s->parameters, even though that is not
> > the case :-) The has_* fields are only used when the various migration
> > QMP methods are executed, and those all use a separate MigrationParameters
> > struct instance.
> 
> Not keeping the has_ members up-to-date is harmless as long as you don't
> pass the thing to visitors, including the one hiding in qapi_free_FOO().
> That one ignores scalars, though.
> 
> 
> From a more abstract point of view, we have two related data types: one
> for the state, and one for state changes requests.
> 
> In state, members are always present.
> 
> A state change request is a bag of state member change requests, and
> each request can either specify the new value or ask for a reset to
> default.  Absent member means no change.
> 
> We press the same QAPI type into service for both by making all members
> optional.
> 
> For the state case, we hardwire the has_ to true.  Or even ignore them
> completely.
> 
> For the state change request, we use has_ = false for "no change", has_
> = true with a special value for "reset to default" (new in this patch)
> and has_ = true with a non-special value for "set to this value".
> 
> Requires a special value outside the set of non-special values.  The
> obvious one is JSON null, but the QAPI generator doesn't quite support
> that, yet.  "" works here, but is not general.
> 
> I think I can get you null support in 2.10.  Would that work for you?

Not really - we need this fix so libvirt can enable TLS migration,
in addition we'll be wanting to add it to stable branches upstream
and downstream.

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/ :|

Re: [Qemu-devel] [PATCH] migration: allow clearing migration string parameters

2017-03-03 Thread Markus Armbruster 
"Daniel P. Berrange"  writes:

> On Wed, Mar 01, 2017 at 08:36:03AM -0600, Eric Blake wrote:
>> On 03/01/2017 06:32 AM, Daniel P. Berrange wrote:
>> >  }
>> >  if (params->has_tls_creds) {
>> >  g_free(s->parameters.tls_creds);
>> > -s->parameters.tls_creds = g_strdup(params->tls_creds);
>> > +if (*params->tls_creds == '\0') {
>> > +s->parameters.tls_creds = NULL;
>> 
>> I'm wondering if you should also do s->parameters.has_tls_creds = false
>> at this point?  The visitors expect that if has_tls_creds is true, then
>> the string is non-NULL.
>
> The fact that s->parameters contains has_* fields is completely ignored
> by the migration code afaict. IOW the code behaves as if all the has_*
> fields are hardwired to true in s->parameters, even though that is not
> the case :-) The has_* fields are only used when the various migration
> QMP methods are executed, and those all use a separate MigrationParameters
> struct instance.

Not keeping the has_ members up-to-date is harmless as long as you don't
pass the thing to visitors, including the one hiding in qapi_free_FOO().
That one ignores scalars, though.


>From a more abstract point of view, we have two related data types: one
for the state, and one for state changes requests.

In state, members are always present.

A state change request is a bag of state member change requests, and
each request can either specify the new value or ask for a reset to
default.  Absent member means no change.

We press the same QAPI type into service for both by making all members
optional.

For the state case, we hardwire the has_ to true.  Or even ignore them
completely.

For the state change request, we use has_ = false for "no change", has_
= true with a special value for "reset to default" (new in this patch)
and has_ = true with a non-special value for "set to this value".

Requires a special value outside the set of non-special values.  The
obvious one is JSON null, but the QAPI generator doesn't quite support
that, yet.  "" works here, but is not general.

I think I can get you null support in 2.10.  Would that work for you?

Re: [Qemu-devel] [PATCH] migration: allow clearing migration string parameters

2017-03-02 Thread Daniel P. Berrange 
On Wed, Mar 01, 2017 at 08:36:03AM -0600, Eric Blake wrote:
> On 03/01/2017 06:32 AM, Daniel P. Berrange wrote:
> >  }
> >  if (params->has_tls_creds) {
> >  g_free(s->parameters.tls_creds);
> > -s->parameters.tls_creds = g_strdup(params->tls_creds);
> > +if (*params->tls_creds == '\0') {
> > +s->parameters.tls_creds = NULL;
> 
> I'm wondering if you should also do s->parameters.has_tls_creds = false
> at this point?  The visitors expect that if has_tls_creds is true, then
> the string is non-NULL.

The fact that s->parameters contains has_* fields is completely ignored
by the migration code afaict. IOW the code behaves as if all the has_*
fields are hardwired to true in s->parameters, even though that is not
the case :-) The has_* fields are only used when the various migration
QMP methods are executed, and those all use a separate MigrationParameters
struct instance. 


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/ :|

Re: [Qemu-devel] [PATCH] migration: allow clearing migration string parameters

2017-03-01 Thread Markus Armbruster 
"Daniel P. Berrange"  writes:

> On Wed, Mar 01, 2017 at 08:36:03AM -0600, Eric Blake wrote:
>> On 03/01/2017 06:32 AM, Daniel P. Berrange wrote:
>> > 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,
>> 
>> s/ommit/omit/
>> 
>> > 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.
>> 
>> That happens to work in this case, because an empty string as the TLS
>> parameter makes no sense.

Yes.

>> A nicer solution would be to support JSON null on the wire, so the user
>> can pass "tls-creds":null to explicitly request wiping out the
>> credentials rather than making the empty string magic.  But that's more
>> invasive (the QAPI parser would have to be enhanced to allow an
>> alternate that visits either a string or null).

We already support JSON null on the wire, we just lack a built-in null
type in the schema language.  It's not hard; sketch appended.  I expect
to have missed a few more checks for the null type.

>>  Ultimately, I think
>> we'll have to do that when we come up with some string property where ""
>> should not be given magic treatment.
>> 
>> So now we have a dilemma: do we special case "" here, to be stuck with
>> it even if we later add nullable-string support later, or do we go all
>> the way to nullable-string support now?  We've missed soft freeze for
>> 2.9, so my vote is that it is okay to special case "" in  this instance,
>> even though we'll have to continue to support it indefinitely even if we
>> gain nullable-string QMP.
>
> FYI, if QEMU ever sends a QMP reply whose value comes from a NULL string,
> it turns that into "". IOW, on output "" and NULL are indistinguishable

C "" != C NULL != JSON null

Let me elaborate.

We use QObject * to represent a JSON expression.

The JSON expression that consists of the literal name null is
represented as a non-null QObject *, created with qnull().

A null QObject * could mean something like "I have no JSON expression
for you", which is not the same as "I'm handing you the JSON expression
consisting of the literal null".

When a QObject * represents a JSON expression, none of its QObject *
children may be null.  Absent JSON object members are simply absent in
the QDict.  There is no such thing as an absent list member.

A QString wraps a char * that cannot be null.  A QString * still can,
like any other QObject *.

The QObject input visitor converts a QObject representing a JSON
expression to QAPI object.  It assumes the QObject * are all non-null,
as are the QString's char *.  It guards its assumptions with assertions.

The QObject output visitor converts a QAPI object to a QObject
representing a JSON expression.  Corresponding assumptions about the
pointers within the QAPI object:

* A pointer to a QAPI object can only be null when it represents an
  optional member that is absent.

* A char * can't be null.

However, we relax the second assumption to map NULL to "" silently.  Bad
idea going back to the early days of QAPI.  We intend to get rid of it,
see TODO comment in visit_type_str().

> So from that POV, treating JSON nil and "" as indistiguishable on input
> would be consistent with what we do on output.

Consistent with a bad idea we want to get rid of, plus external
exposure, so we can't :)

> I'd only suggest allowing JSON nil on input, if we're willing change the
> output code to map NULL -> nil, instead of NULL -> "".

No, the mapping is between JSON null and QNull *.  C null pointers are
something else entirely.

Back when I fitted support for JSON null into this code, I considered
representing it as NULL, but quickly gave up due to the existing usage
of NULL for something else entirely.




diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index dabc42e..939d1e7 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -41,6 +41,8 @@ struct %(c_name)s {
 def gen_struct_members(members):
 ret = ''
 for memb in members:
+if memb.type.c_type() is None:
+continue
 if memb.optional:
 ret += mcgen('''

Re: [Qemu-devel] [PATCH] migration: allow clearing migration string parameters

2017-03-01 Thread Eric Blake 
On 03/01/2017 08:48 AM, Daniel P. Berrange wrote:

>>
>> So now we have a dilemma: do we special case "" here, to be stuck with
>> it even if we later add nullable-string support later, or do we go all
>> the way to nullable-string support now?  We've missed soft freeze for
>> 2.9, so my vote is that it is okay to special case "" in  this instance,
>> even though we'll have to continue to support it indefinitely even if we
>> gain nullable-string QMP.
> 
> FYI, if QEMU ever sends a QMP reply whose value comes from a NULL string,
> it turns that into "". IOW, on output "" and NULL are indistinguishable

That's a bug. My work to add a JSON output visitor requires a non-null
string (it is only the qobject output visitor that silently converts
NULL into "", and only because we haven't yet audited the broken
callers).  We WANT to reach the point where "" must be supplied when it
is intended, and where a NULL string can be output as JSON null where
appropriate.

> 
> So from that POV, treating JSON nil and "" as indistiguishable on input
> would be consistent with what we do on output.

What we do on output is a result of laziness, not of user input.
Codifying "" as NULL on user input is different than the existing
treatment of NULL as "" on output.

> 
> I'd only suggest allowing JSON nil on input, if we're willing change the
> output code to map NULL -> nil, instead of NULL -> "".

And yes, I think there ARE cases where we probably want to allow output
of JSON null (probably in the same contexts where we want to allow null
on input).

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH] migration: allow clearing migration string parameters

2017-03-01 Thread Daniel P. Berrange 
On Wed, Mar 01, 2017 at 08:36:03AM -0600, Eric Blake wrote:
> On 03/01/2017 06:32 AM, Daniel P. Berrange wrote:
> > 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,
> 
> s/ommit/omit/
> 
> > 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.
> 
> That happens to work in this case, because an empty string as the TLS
> parameter makes no sense.
> 
> A nicer solution would be to support JSON null on the wire, so the user
> can pass "tls-creds":null to explicitly request wiping out the
> credentials rather than making the empty string magic.  But that's more
> invasive (the QAPI parser would have to be enhanced to allow an
> alternate that visits either a string or null).  Ultimately, I think
> we'll have to do that when we come up with some string property where ""
> should not be given magic treatment.
> 
> So now we have a dilemma: do we special case "" here, to be stuck with
> it even if we later add nullable-string support later, or do we go all
> the way to nullable-string support now?  We've missed soft freeze for
> 2.9, so my vote is that it is okay to special case "" in  this instance,
> even though we'll have to continue to support it indefinitely even if we
> gain nullable-string QMP.

FYI, if QEMU ever sends a QMP reply whose value comes from a NULL string,
it turns that into "". IOW, on output "" and NULL are indistinguishable

So from that POV, treating JSON nil and "" as indistiguishable on input
would be consistent with what we do on output.

I'd only suggest allowing JSON nil on input, if we're willing change the
output code to map NULL -> nil, instead of NULL -> "".

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/ :|

Re: [Qemu-devel] [PATCH] migration: allow clearing migration string parameters

2017-03-01 Thread Eric Blake 
On 03/01/2017 06:32 AM, Daniel P. Berrange wrote:
> 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,

s/ommit/omit/

> 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.

That happens to work in this case, because an empty string as the TLS
parameter makes no sense.

A nicer solution would be to support JSON null on the wire, so the user
can pass "tls-creds":null to explicitly request wiping out the
credentials rather than making the empty string magic.  But that's more
invasive (the QAPI parser would have to be enhanced to allow an
alternate that visits either a string or null).  Ultimately, I think
we'll have to do that when we come up with some string property where ""
should not be given magic treatment.

So now we have a dilemma: do we special case "" here, to be stuck with
it even if we later add nullable-string support later, or do we go all
the way to nullable-string support now?  We've missed soft freeze for
2.9, so my vote is that it is okay to special case "" in  this instance,
even though we'll have to continue to support it indefinitely even if we
gain nullable-string QMP.

> 
> Signed-off-by: Daniel P. Berrange 
> ---
> 
> CC'ing Eric & Markus since this is related to QAPI schema for migrate
> monitor commands
> 

>  }
>  if (params->has_tls_creds) {
>  g_free(s->parameters.tls_creds);
> -s->parameters.tls_creds = g_strdup(params->tls_creds);
> +if (*params->tls_creds == '\0') {
> +s->parameters.tls_creds = NULL;

I'm wondering if you should also do s->parameters.has_tls_creds = false
at this point?  The visitors expect that if has_tls_creds is true, then
the string is non-NULL.

> +} else {
> +s->parameters.tls_creds = g_strdup(params->tls_creds);
> +}
>  }
>  if (params->has_tls_hostname) {
>  g_free(s->parameters.tls_hostname);
> -s->parameters.tls_hostname = g_strdup(params->tls_hostname);
> +if (*params->tls_hostnane == '\0') {
> +s->parameters.tls_hostnane = NULL;

This build-breaking typo isn't intended. :)

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH] migration: allow clearing migration string parameters

2017-03-01 Thread no-reply 
Hi,

This series failed build test on s390x host. Please find the details below.

Message-id: 20170301123223.12489-1-berra...@redhat.com
Type: series
Subject: [Qemu-devel] [PATCH] migration: allow clearing migration string 
parameters

=== TEST SCRIPT BEGIN ===
#!/bin/bash
# Testing script will be invoked under the git checkout with
# HEAD pointing to a commit that has the patches applied on top of "base"
# branch
set -e
echo "=== ENV ==="
env
echo "=== PACKAGES ==="
rpm -qa
echo "=== TEST BEGIN ==="
CC=$HOME/bin/cc
INSTALL=$PWD/install
BUILD=/var/tmp/patchew-qemu-build
echo -n "Using CC: "
realpath $CC
test -e $BUILD && rm -rf $BUILD
mkdir -p $BUILD $INSTALL
SRC=$PWD
cd $BUILD
$SRC/configure --cc=$CC --prefix=$INSTALL
make -j4
make check -j4
make install
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 - [tag update]  patchew/20170301115026.22621-1-stefa...@redhat.com -> 
patchew/20170301115026.22621-1-stefa...@redhat.com
 * [new tag] patchew/20170301123223.12489-1-berra...@redhat.com -> 
patchew/20170301123223.12489-1-berra...@redhat.com
Switched to a new branch 'test'
fa5c7b6 migration: allow clearing migration string parameters

=== OUTPUT BEGIN ===
=== ENV ===
XDG_SESSION_ID=41452
SHELL=/bin/sh
USER=fam
PATCHEW=/home/fam/patchew/patchew-cli -s http://patchew.org --nodebug
PATH=/usr/bin:/bin
PWD=/var/tmp/patchew-tester-tmp-sanhpv5m/src
LANG=en_US.UTF-8
HOME=/home/fam
SHLVL=2
LOGNAME=fam
DBUS_SESSION_BUS_ADDRESS=unix:path=/run/user/1012/bus
XDG_RUNTIME_DIR=/run/user/1012
_=/usr/bin/env
=== PACKAGES ===
gpg-pubkey-873529b8-54e386ff
xz-libs-5.2.2-2.fc24.s390x
libacl-2.2.52-11.fc24.s390x
libxshmfence-1.2-3.fc24.s390x
cdparanoia-libs-10.2-21.fc24.s390x
ustr-1.0.4-21.fc24.s390x
giflib-4.1.6-15.fc24.s390x
libusb-0.1.5-7.fc24.s390x
trousers-lib-0.3.13-6.fc24.s390x
readline-devel-6.3-8.fc24.s390x
python-srpm-macros-3-10.fc25.noarch
ncurses-base-6.0-6.20160709.fc25.noarch
gmp-6.1.1-1.fc25.s390x
chkconfig-1.8-1.fc25.s390x
libidn-1.33-1.fc25.s390x
file-5.28-4.fc25.s390x
slang-2.3.0-7.fc25.s390x
avahi-libs-0.6.32-4.fc25.s390x
libsemanage-2.5-8.fc25.s390x
perl-Unicode-Normalize-1.25-365.fc25.s390x
perl-libnet-3.10-1.fc25.noarch
perl-Thread-Queue-3.11-1.fc25.noarch
perl-podlators-4.09-1.fc25.noarch
jasper-libs-1.900.13-1.fc25.s390x
graphite2-1.3.6-1.fc25.s390x
libblkid-2.28.2-1.fc25.s390x
pkgconfig-0.29.1-1.fc25.s390x
dbus-python-1.2.4-2.fc25.s390x
alsa-lib-1.1.1-2.fc25.s390x
libgnome-keyring-3.12.0-7.fc25.s390x
yum-metadata-parser-1.1.4-17.fc25.s390x
python3-3.5.2-4.fc25.s390x
python3-slip-dbus-0.6.4-4.fc25.noarch
python2-cssselect-0.9.2-1.fc25.noarch
python-backports-1.0-8.fc25.s390x
python-magic-5.28-4.fc25.noarch
python-pycparser-2.14-7.fc25.noarch
python-fedora-0.8.0-2.fc25.noarch
createrepo_c-libs-0.10.0-6.fc25.s390x
initscripts-9.69-1.fc25.s390x
plymouth-scripts-0.9.3-0.6.20160620git0e65b86c.fc25.s390x
cronie-1.5.1-2.fc25.s390x
python2-librepo-1.7.18-3.fc25.s390x
wget-1.18-2.fc25.s390x
python3-dnf-plugins-core-0.1.21-4.fc25.noarch
at-spi2-core-2.22.0-1.fc25.s390x
libXv-1.0.11-1.fc25.s390x
dhcp-client-4.3.5-1.fc25.s390x
python2-dnf-plugins-core-0.1.21-4.fc25.noarch
parted-3.2-21.fc25.s390x
python2-ndg_httpsclient-0.4.0-4.fc25.noarch
bash-completion-2.4-1.fc25.noarch
btrfs-progs-4.6.1-1.fc25.s390x
texinfo-6.1-3.fc25.s390x
perl-Filter-1.55-366.fc25.s390x
flex-2.6.0-3.fc25.s390x
libgcc-6.3.1-1.fc25.s390x
glib2-2.50.2-1.fc25.s390x
dbus-libs-1.11.8-1.fc25.s390x
libgomp-6.3.1-1.fc25.s390x
colord-libs-1.3.4-1.fc25.s390x
perl-Encode-2.88-5.fc25.s390x
gstreamer1-1.10.2-1.fc25.s390x
cracklib-2.9.6-4.fc25.s390x
rpm-build-libs-4.13.0-6.fc25.s390x
libobjc-6.3.1-1.fc25.s390x
pcre-devel-8.40-1.fc25.s390x
mariadb-config-10.1.20-1.fc25.s390x
gcc-6.3.1-1.fc25.s390x
mesa-libGL-13.0.3-1.fc25.s390x
python3-dnf-plugin-system-upgrade-0.7.1-4.fc25.noarch
bind-libs-9.10.4-4.P5.fc25.s390x
python-osbs-client-0.33-3.fc25.noarch
NetworkManager-1.4.4-3.fc25.s390x
audit-2.7.1-1.fc25.s390x
glibc-static-2.24-4.fc25.s390x
perl-Pod-Simple-3.35-1.fc25.noarch
gdb-7.12-36.fc25.s390x
python2-simplejson-3.10.0-1.fc25.s390x
python3-sssdconfig-1.14.2-2.fc25.noarch
texlive-lib-2016-30.20160520.fc25.s390x
boost-random-1.60.0-10.fc25.s390x
brltty-5.4-2.fc25.s390x
libref_array-0.1.5-29.fc25.s390x
librados2-10.2.4-2.fc25.s390x
gnutls-dane-3.5.8-1.fc25.s390x
systemtap-client-3.1-0.20160725git91bfb36.fc25.s390x
libXrender-devel-0.9.10-1.fc25.s390x
libXi-devel-1.7.8-2.fc25.s390x
texlive-pdftex-doc-svn41149-30.fc25.noarch
tcp_wrappers-7.6-83.fc25.s390x
javapackages-tools-4.7.0-6.1.fc25.noarch
texlive-kpathsea-bin-svn40473-30.20160520.fc25.s390x
texlive-url-svn32528.3.4-30.fc25.noarch
texlive-latex-fonts-svn2.0-30.fc25.noarch
texlive-mptopdf-bin-svn18674.0-30.20160520.fc25.noarch
texlive-underscore-svn18261.0-30.fc25.noarch
texlive-subfig-svn15878.1.3-30.fc25.noarch
texlive-dvipdfmx-def-svn40328-30.fc25.noar

Re: [Qemu-devel] [PATCH] migration: allow clearing migration string parameters

2017-03-01 Thread no-reply 
Hi,

This series failed automatic build test. Please find the testing commands and
their output below. If you have docker installed, you can probably reproduce it
locally.

Message-id: 20170301123223.12489-1-berra...@redhat.com
Type: series
Subject: [Qemu-devel] [PATCH] migration: allow clearing migration string 
parameters

=== TEST SCRIPT BEGIN ===
#!/bin/bash
set -e
git submodule update --init dtc
# Let docker tests dump environment info
export SHOW_ENV=1
export J=16
make docker-test-quick@centos6
make docker-test-mingw@fedora
make docker-test-build@min-glib
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
fa5c7b6 migration: allow clearing migration string parameters

=== OUTPUT BEGIN ===
Submodule 'dtc' (git://git.qemu-project.org/dtc.git) registered for path 'dtc'
Cloning into 'dtc'...
Submodule path 'dtc': checked out 'ec02b34c05be04f249ffaaca4b666f5246877dea'
  BUILD   centos6
make[1]: Entering directory `/var/tmp/patchew-tester-tmp-o6x8jap0/src'
  ARCHIVE qemu.tgz
  ARCHIVE dtc.tgz
  COPYRUNNER
RUN test-quick in qemu:centos6 
Packages installed:
SDL-devel-1.2.14-7.el6_7.1.x86_64
ccache-3.1.6-2.el6.x86_64
epel-release-6-8.noarch
gcc-4.4.7-17.el6.x86_64
git-1.7.1-4.el6_7.1.x86_64
glib2-devel-2.28.8-5.el6.x86_64
libfdt-devel-1.4.0-1.el6.x86_64
make-3.81-23.el6.x86_64
package g++ is not installed
pixman-devel-0.32.8-1.el6.x86_64
tar-1.23-15.el6_8.x86_64
zlib-devel-1.2.3-29.el6.x86_64

Environment variables:
PACKAGES=libfdt-devel ccache tar git make gcc g++ zlib-devel 
glib2-devel SDL-devel pixman-devel epel-release
HOSTNAME=51da1d571f8b
TERM=xterm
MAKEFLAGS= -j16
HISTSIZE=1000
J=16
USER=root
CCACHE_DIR=/var/tmp/ccache
EXTRA_CONFIGURE_OPTS=
V=
SHOW_ENV=1
MAIL=/var/spool/mail/root
PATH=/usr/lib/ccache:/usr/lib64/ccache:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
PWD=/
LANG=en_US.UTF-8
TARGET_LIST=
HISTCONTROL=ignoredups
SHLVL=1
HOME=/root
TEST_DIR=/tmp/qemu-test
LOGNAME=root
LESSOPEN=||/usr/bin/lesspipe.sh %s
FEATURES= dtc
DEBUG=
G_BROKEN_FILENAMES=1
CCACHE_HASHDIR=
_=/usr/bin/env

Configure options:
--enable-werror --target-list=x86_64-softmmu,aarch64-softmmu 
--prefix=/var/tmp/qemu-build/install
No C++ compiler available; disabling C++ specific optional code
Install prefix/var/tmp/qemu-build/install
BIOS directory/var/tmp/qemu-build/install/share/qemu
binary directory  /var/tmp/qemu-build/install/bin
library directory /var/tmp/qemu-build/install/lib
module directory  /var/tmp/qemu-build/install/lib/qemu
libexec directory /var/tmp/qemu-build/install/libexec
include directory /var/tmp/qemu-build/install/include
config directory  /var/tmp/qemu-build/install/etc
local state directory   /var/tmp/qemu-build/install/var
Manual directory  /var/tmp/qemu-build/install/share/man
ELF interp prefix /usr/gnemul/qemu-%M
Source path   /tmp/qemu-test/src
C compilercc
Host C compiler   cc
C++ compiler  
Objective-C compiler cc
ARFLAGS   rv
CFLAGS-O2 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -g 
QEMU_CFLAGS   -I/usr/include/pixman-1   -I$(SRC_PATH)/dtc/libfdt -pthread 
-I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include   -fPIE -DPIE -m64 -mcx16 
-D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes 
-Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes 
-fno-strict-aliasing -fno-common -fwrapv  -Wendif-labels 
-Wno-missing-include-dirs -Wempty-body -Wnested-externs -Wformat-security 
-Wformat-y2k -Winit-self -Wignored-qualifiers -Wold-style-declaration 
-Wold-style-definition -Wtype-limits -fstack-protector-all
LDFLAGS   -Wl,--warn-common -Wl,-z,relro -Wl,-z,now -pie -m64 -g 
make  make
install   install
pythonpython -B
smbd  /usr/sbin/smbd
module supportno
host CPU  x86_64
host big endian   no
target list   x86_64-softmmu aarch64-softmmu
tcg debug enabled no
gprof enabled no
sparse enabledno
strip binariesyes
profiler  no
static build  no
pixmansystem
SDL support   yes (1.2.14)
GTK support   no 
GTK GL supportno
VTE support   no 
TLS priority  NORMAL
GNUTLS supportno
GNUTLS rndno
libgcrypt no
libgcrypt kdf no
nettleno 
nettle kdfno
libtasn1  no
curses supportno
virgl support no
curl support  no
mingw32 support   no
Audio drivers oss
Block whitelist (rw) 
Block whitelist (ro) 
VirtFS supportno
VNC support   yes
VNC SASL support  no
VNC JPEG support  no
VNC PNG support   no
xen support   no
brlapi supportno
bluez  supportno
Documentation no
PIE   yes
vde support   no
netmap supportno
Linux AIO support no
ATTR/XATTR support yes
Install blobs yes
KVM support   yes
HAX support   no
RDMA support  no
TCG interpreter   no
fdt support   yes
preadv supportyes
fdatasync yes
madvise

[Qemu-devel] [PATCH] migration: allow clearing migration string parameters

2017-03-01 Thread Daniel P. Berrange 
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.

Signed-off-by: Daniel P. Berrange 
---

CC'ing Eric & Markus since this is related to QAPI schema for migrate
monitor commands

 migration/migration.c | 12 ++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index c6ae69d..a5d41a9 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -872,11 +872,19 @@ void qmp_migrate_set_parameters(MigrationParameters 
*params, Error **errp)
 }
 if (params->has_tls_creds) {
 g_free(s->parameters.tls_creds);
-s->parameters.tls_creds = g_strdup(params->tls_creds);
+if (*params->tls_creds == '\0') {
+s->parameters.tls_creds = NULL;
+} else {
+s->parameters.tls_creds = g_strdup(params->tls_creds);
+}
 }
 if (params->has_tls_hostname) {
 g_free(s->parameters.tls_hostname);
-s->parameters.tls_hostname = g_strdup(params->tls_hostname);
+if (*params->tls_hostnane == '\0') {
+s->parameters.tls_hostnane = NULL;
+} else {
+s->parameters.tls_hostname = g_strdup(params->tls_hostname);
+}
 }
 if (params->has_max_bandwidth) {
 s->parameters.max_bandwidth = params->max_bandwidth;
-- 
2.9.3