| Message ID | 1495649128-10529-7-git-send-email-vyasevic@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 05/24/2017 01:05 PM, Vladislav Yasevich wrote: > Add a qmp command that can trigger guest announcements. > > Based on work of Germano Veit Michel <germano@redhat.com> > > Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com> > --- > migration/savevm.c | 14 ++++++++++++++ > qapi-schema.json | 19 +++++++++++++++++++ > 2 files changed, 33 insertions(+) > > +void qmp_announce_self(bool has_params, AnnounceParameters *params, > + Error **errp) > +{ > + AnnounceParameters announce_params; > + > + memcpy(&announce_params, qemu_get_announce_params(), > + sizeof(announce_params)); Shallow copies of a QAPI type happen to work when the type is all scalar (as AnnounceParameters currently is), but it's more robust to use QAPI_CLONE() or QAPI_CLONE_MEMBERS() so that any future non-scalar additions to the parameters type will be correctly copied without introducing memory leaks or double frees. Even this might be better: AnnounceParameters announce_params = { 0 }; qemu_set_announce_parameters(&announce_params, qemu_get_announce_params()); > + > + if (has_params) > + qemu_set_announce_parameters(&announce_params, params); > + > + qemu_announce_self(&announce_params); > +} The QMP changes look okay. Do you have a testsuite covering the new QMP command somewhere in the series?
On 05/26/2017 09:16 AM, Eric Blake wrote: > On 05/24/2017 01:05 PM, Vladislav Yasevich wrote: >> Add a qmp command that can trigger guest announcements. >> >> Based on work of Germano Veit Michel <germano@redhat.com> >> >> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com> >> --- >> migration/savevm.c | 14 ++++++++++++++ >> qapi-schema.json | 19 +++++++++++++++++++ >> 2 files changed, 33 insertions(+) >> > >> +void qmp_announce_self(bool has_params, AnnounceParameters *params, >> + Error **errp) >> +{ >> + AnnounceParameters announce_params; >> + >> + memcpy(&announce_params, qemu_get_announce_params(), >> + sizeof(announce_params)); > > Shallow copies of a QAPI type happen to work when the type is all scalar > (as AnnounceParameters currently is), but it's more robust to use > QAPI_CLONE() or QAPI_CLONE_MEMBERS() so that any future non-scalar > additions to the parameters type will be correctly copied without > introducing memory leaks or double frees. > > Even this might be better: > AnnounceParameters announce_params = { 0 }; > qemu_set_announce_parameters(&announce_params, qemu_get_announce_params()); > Ok. >> + >> + if (has_params) >> + qemu_set_announce_parameters(&announce_params, params); >> + >> + qemu_announce_self(&announce_params); >> +} > The QMP changes look okay. Do you have a testsuite covering the new QMP > command somewhere in the series? > There is basic test in patch 12. Thanks -vlad
Vladislav Yasevich <vyasevic@redhat.com> wrote: > Add a qmp command that can trigger guest announcements. > > Based on work of Germano Veit Michel <germano@redhat.com> > > Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com> > --- > migration/savevm.c | 14 ++++++++++++++ > qapi-schema.json | 19 +++++++++++++++++++ > 2 files changed, 33 insertions(+) > > diff --git a/migration/savevm.c b/migration/savevm.c > index a4097c9..b55ce6a 100644 > --- a/migration/savevm.c > +++ b/migration/savevm.c > @@ -265,6 +265,20 @@ void qemu_announce_self(AnnounceParameters *params) > qemu_announce_self_once(timer); > } > > +void qmp_announce_self(bool has_params, AnnounceParameters *params, > + Error **errp) > +{ > + AnnounceParameters announce_params; > + > + memcpy(&announce_params, qemu_get_announce_params(), > + sizeof(announce_params)); > + > + if (has_params) > + qemu_set_announce_parameters(&announce_params, params); > + > + qemu_announce_self(&announce_params); Are I missreading qemu_annouce_self()? My reading is that it passes announce_params to a timer (i.e. async function), but here announce_params is a local variable here, no?
On 05/30/2017 06:11 AM, Juan Quintela wrote: > Vladislav Yasevich <vyasevic@redhat.com> wrote: >> Add a qmp command that can trigger guest announcements. >> >> Based on work of Germano Veit Michel <germano@redhat.com> >> >> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com> >> --- >> migration/savevm.c | 14 ++++++++++++++ >> qapi-schema.json | 19 +++++++++++++++++++ >> 2 files changed, 33 insertions(+) >> >> diff --git a/migration/savevm.c b/migration/savevm.c >> index a4097c9..b55ce6a 100644 >> --- a/migration/savevm.c >> +++ b/migration/savevm.c >> @@ -265,6 +265,20 @@ void qemu_announce_self(AnnounceParameters *params) >> qemu_announce_self_once(timer); >> } >> >> +void qmp_announce_self(bool has_params, AnnounceParameters *params, >> + Error **errp) >> +{ >> + AnnounceParameters announce_params; >> + >> + memcpy(&announce_params, qemu_get_announce_params(), >> + sizeof(announce_params)); >> + >> + if (has_params) >> + qemu_set_announce_parameters(&announce_params, params); >> + >> + qemu_announce_self(&announce_params); > > Are I missreading qemu_annouce_self()? > My reading is that it passes announce_params to a timer (i.e. async > function), but here announce_params is a local variable here, no? > The AnnounceTimer holds a copy since each timer may have it's own values. Thanks -vlad
On Wed, May 24, 2017 at 02:05:22PM -0400, Vladislav Yasevich wrote: > Add a qmp command that can trigger guest announcements. > > Based on work of Germano Veit Michel <germano@redhat.com> > > Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com> > --- > migration/savevm.c | 14 ++++++++++++++ > qapi-schema.json | 19 +++++++++++++++++++ > 2 files changed, 33 insertions(+) > > diff --git a/migration/savevm.c b/migration/savevm.c > index a4097c9..b55ce6a 100644 > --- a/migration/savevm.c > +++ b/migration/savevm.c > @@ -265,6 +265,20 @@ void qemu_announce_self(AnnounceParameters *params) > qemu_announce_self_once(timer); > } > > +void qmp_announce_self(bool has_params, AnnounceParameters *params, > + Error **errp) > +{ > + AnnounceParameters announce_params; > + > + memcpy(&announce_params, qemu_get_announce_params(), > + sizeof(announce_params)); > + > + if (has_params) > + qemu_set_announce_parameters(&announce_params, params); > + > + qemu_announce_self(&announce_params); > +} I'm not a huge fan of the idea of setting announce parameters in a global namespace, via a previously issued separate command. I realize this is already done for the 'migrate' command, but there it has been the cause of a number of bugs in libvirt mgmt of QEMU. At least with migrate it makes sense for some parameters which need to be tuned 'on the fly' after migrate already started. That doesn't apply to this command though. So I tend to think it'd be nicer to just make the parameters mandatory for this command, so it is self-contanied does and not rely on previously set (or potentially unknown/indeterminate) global state. I'd also suggest that instad of adding an 'announce_set_parameters', the parameters for the automatic announce at end of migration should be set via the 'migrate_set_parameters' command. > diff --git a/qapi-schema.json b/qapi-schema.json > index 2030087..126b09d 100644 > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -654,6 +654,25 @@ > 'returns': 'AnnounceParameters' } > > ## > +# @announce-self: > +# > +# Trigger generation of broadcast RARP frames to update network switches. > +# This can be useful when network bonds fail-over the active slave. > +# > +# Arguments: None. "None", but it has some arguments listed right below.... > +# > +# Example: > +# > +# -> { "execute": "announce-self" > +# "arguments": { "announce-rounds": 5 } } > +# <- { "return": {} } > +# > +# Since: 2.10 > +## > +{ 'command': 'announce-self', > + 'data' : {'*params': 'AnnounceParameters'} } > + > +## > # @MigrationStats: > # > # Detailed migration status. > -- > 2.7.4 > > Regards, Daniel
Vlad Yasevich <vyasevic@redhat.com> wrote: > On 05/30/2017 06:11 AM, Juan Quintela wrote: >> Vladislav Yasevich <vyasevic@redhat.com> wrote: >>> Add a qmp command that can trigger guest announcements. >>> >>> Based on work of Germano Veit Michel <germano@redhat.com> >>> >>> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com> >>> --- >>> migration/savevm.c | 14 ++++++++++++++ >>> qapi-schema.json | 19 +++++++++++++++++++ >>> 2 files changed, 33 insertions(+) >>> >>> diff --git a/migration/savevm.c b/migration/savevm.c >>> index a4097c9..b55ce6a 100644 >>> --- a/migration/savevm.c >>> +++ b/migration/savevm.c >>> @@ -265,6 +265,20 @@ void qemu_announce_self(AnnounceParameters *params) >>> qemu_announce_self_once(timer); >>> } >>> >>> +void qmp_announce_self(bool has_params, AnnounceParameters *params, >>> + Error **errp) >>> +{ >>> + AnnounceParameters announce_params; >>> + >>> + memcpy(&announce_params, qemu_get_announce_params(), >>> + sizeof(announce_params)); >>> + >>> + if (has_params) >>> + qemu_set_announce_parameters(&announce_params, params); >>> + >>> + qemu_announce_self(&announce_params); >> >> Are I missreading qemu_annouce_self()? >> My reading is that it passes announce_params to a timer (i.e. async >> function), but here announce_params is a local variable here, no? >> > > The AnnounceTimer holds a copy since each timer may have it's own values. > AnnounceTimer *qemu_announce_timer_new(AnnounceParameters *params, > QEMUClockType type) > { > AnnounceTimer *timer = g_new(AnnounceTimer, 1); > > timer->params = *params; I have to remomember that C has learn how to copy structures long ago. <repeat myself one hundred times> I was missing the "*" on my previous reading, sorry for the noise. > timer->round = params->rounds; > timer->type = type; > > return timer; >}
On 05/30/2017 10:24 AM, Juan Quintela wrote: > Vlad Yasevich <vyasevic@redhat.com> wrote: >> On 05/30/2017 06:11 AM, Juan Quintela wrote: >>> Vladislav Yasevich <vyasevic@redhat.com> wrote: >>>> Add a qmp command that can trigger guest announcements. >>>> >>>> Based on work of Germano Veit Michel <germano@redhat.com> >>>> >>>> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com> >>>> --- >>>> migration/savevm.c | 14 ++++++++++++++ >>>> qapi-schema.json | 19 +++++++++++++++++++ >>>> 2 files changed, 33 insertions(+) >>>> >>>> diff --git a/migration/savevm.c b/migration/savevm.c >>>> index a4097c9..b55ce6a 100644 >>>> --- a/migration/savevm.c >>>> +++ b/migration/savevm.c >>>> @@ -265,6 +265,20 @@ void qemu_announce_self(AnnounceParameters *params) >>>> qemu_announce_self_once(timer); >>>> } >>>> >>>> +void qmp_announce_self(bool has_params, AnnounceParameters *params, >>>> + Error **errp) >>>> +{ >>>> + AnnounceParameters announce_params; >>>> + >>>> + memcpy(&announce_params, qemu_get_announce_params(), >>>> + sizeof(announce_params)); >>>> + >>>> + if (has_params) >>>> + qemu_set_announce_parameters(&announce_params, params); >>>> + >>>> + qemu_announce_self(&announce_params); >>> >>> Are I missreading qemu_annouce_self()? >>> My reading is that it passes announce_params to a timer (i.e. async >>> function), but here announce_params is a local variable here, no? >>> >> >> The AnnounceTimer holds a copy since each timer may have it's own values. > > > >> AnnounceTimer *qemu_announce_timer_new(AnnounceParameters *params, >> QEMUClockType type) >> { >> AnnounceTimer *timer = g_new(AnnounceTimer, 1); >> >> timer->params = *params; > > I have to remomember that C has learn how to copy structures long ago. > > <repeat myself one hundred times> > > I was missing the "*" on my previous reading, sorry for the noise. that actually changed, since as Eric Pointed out, shallow copies shouldn't be used. In the v2 code, this uses qemu_set_announce_paramters(), but the copy essentially remains. -vlad > >> timer->round = params->rounds; >> timer->type = type; >> >> return timer; >> } > >
On 05/30/2017 10:14 AM, Daniel P. Berrange wrote: > On Wed, May 24, 2017 at 02:05:22PM -0400, Vladislav Yasevich wrote: >> Add a qmp command that can trigger guest announcements. >> >> Based on work of Germano Veit Michel <germano@redhat.com> >> >> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com> >> --- >> migration/savevm.c | 14 ++++++++++++++ >> qapi-schema.json | 19 +++++++++++++++++++ >> 2 files changed, 33 insertions(+) >> >> diff --git a/migration/savevm.c b/migration/savevm.c >> index a4097c9..b55ce6a 100644 >> --- a/migration/savevm.c >> +++ b/migration/savevm.c >> @@ -265,6 +265,20 @@ void qemu_announce_self(AnnounceParameters *params) >> qemu_announce_self_once(timer); >> } >> >> +void qmp_announce_self(bool has_params, AnnounceParameters *params, >> + Error **errp) >> +{ >> + AnnounceParameters announce_params; >> + >> + memcpy(&announce_params, qemu_get_announce_params(), >> + sizeof(announce_params)); >> + >> + if (has_params) >> + qemu_set_announce_parameters(&announce_params, params); >> + >> + qemu_announce_self(&announce_params); >> +} > > I'm not a huge fan of the idea of setting announce parameters in a > global namespace, via a previously issued separate command. > > I realize this is already done for the 'migrate' command, but there > it has been the cause of a number of bugs in libvirt mgmt of QEMU. > At least with migrate it makes sense for some parameters which need > to be tuned 'on the fly' after migrate already started. That doesn't > apply to this command though. > > So I tend to think it'd be nicer to just make the parameters mandatory > for this command, so it is self-contanied does and not rely on previously > set (or potentially unknown/indeterminate) global state. > The reason I didn't do this is that I didn't want to require the user to know or even think about the timeout values. I did want to provide an option though for those who may want to tweak things. In most circumstances, the timeout values would never really be touched and we'll just use the qemu defaults. in rare circumstances, values might need to be changed during migration. Here, I can definitely see the need to give migration it's own values. > I'd also suggest that instad of adding an 'announce_set_parameters', > the parameters for the automatic announce at end of migration should > be set via the 'migrate_set_parameters' command. I can do that. > >> diff --git a/qapi-schema.json b/qapi-schema.json >> index 2030087..126b09d 100644 >> --- a/qapi-schema.json >> +++ b/qapi-schema.json >> @@ -654,6 +654,25 @@ >> 'returns': 'AnnounceParameters' } >> >> ## >> +# @announce-self: >> +# >> +# Trigger generation of broadcast RARP frames to update network switches. >> +# This can be useful when network bonds fail-over the active slave. >> +# >> +# Arguments: None. > > "None", but it has some arguments listed right below.... will fix Thanks -vlad > >> +# >> +# Example: >> +# >> +# -> { "execute": "announce-self" >> +# "arguments": { "announce-rounds": 5 } } >> +# <- { "return": {} } >> +# >> +# Since: 2.10 >> +## >> +{ 'command': 'announce-self', >> + 'data' : {'*params': 'AnnounceParameters'} } >> + >> +## >> # @MigrationStats: >> # >> # Detailed migration status. >> -- >> 2.7.4 >> >> > > Regards, > Daniel >
diff --git a/migration/savevm.c b/migration/savevm.c index a4097c9..b55ce6a 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -265,6 +265,20 @@ void qemu_announce_self(AnnounceParameters *params) qemu_announce_self_once(timer); } +void qmp_announce_self(bool has_params, AnnounceParameters *params, + Error **errp) +{ + AnnounceParameters announce_params; + + memcpy(&announce_params, qemu_get_announce_params(), + sizeof(announce_params)); + + if (has_params) + qemu_set_announce_parameters(&announce_params, params); + + qemu_announce_self(&announce_params); +} + /***********************************************************/ /* savevm/loadvm support */ diff --git a/qapi-schema.json b/qapi-schema.json index 2030087..126b09d 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -654,6 +654,25 @@ 'returns': 'AnnounceParameters' } ## +# @announce-self: +# +# Trigger generation of broadcast RARP frames to update network switches. +# This can be useful when network bonds fail-over the active slave. +# +# Arguments: None. +# +# Example: +# +# -> { "execute": "announce-self" +# "arguments": { "announce-rounds": 5 } } +# <- { "return": {} } +# +# Since: 2.10 +## +{ 'command': 'announce-self', + 'data' : {'*params': 'AnnounceParameters'} } + +## # @MigrationStats: # # Detailed migration status.
Add a qmp command that can trigger guest announcements. Based on work of Germano Veit Michel <germano@redhat.com> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com> --- migration/savevm.c | 14 ++++++++++++++ qapi-schema.json | 19 +++++++++++++++++++ 2 files changed, 33 insertions(+)