Re: [libvirt] [PATCH v2] qemu: neglect cur_balloon in supplied xml

On 30.07.2016 16:04, John Ferlan wrote: > s/$subj/qemu: Filter cur_balloon ABI check for certain transactions > > On 06/09/2016 10:32 AM, Nikolay Shirokovskiy wrote: >> cur_balloon value can change in between preparing external config and >> using it in operations like save and migrate. As a resutl operation will >> fail for ABI inconsistency. cur_balloon changes can not be predicted >> generally and thus operations will fail from time to time. >> >> Skip checking cur_balloon if domain lock can not be hold between >> preparing external config outside of libvirt and checking it against active >> config. Instead update cur_balloon value in external config from active config. >> This way it is protected from forges and is keeped up to date too. >> > > s/$commit/ > > Since the domain lock is not held during preparation of an external XML > config, it is possible that the value can change resulting in unexpected > failures during ABI consistency checking for some save and migrate > operations. > > This patch adds a new flag to skip the checking of the cur_balloon value > and then sets the destination value to the source value to ensure > subsequent checks without the skip flag will succeed. > >> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com&gt; >> --- >> src/conf/domain_conf.c | 14 +++++++++++--- >> src/conf/domain_conf.h | 9 +++++++++ >> src/libvirt_private.syms | 1 + >> src/qemu/qemu_domain.c | 29 ++++++++++++++++++++--------- >> src/qemu/qemu_domain.h | 6 +++--- >> src/qemu/qemu_driver.c | 5 +++-- >> src/qemu/qemu_migration.c | 4 ++-- >> 7 files changed, 49 insertions(+), 19 deletions(-) >> > > This change seems reasonable to me and it doesn't seem to negate the > primary focus of commit id 'f2fc1eee' (which added the checks). > > I do have a few suggestions and can make those locally before pushing > after the current release is cut. > > Thanx! I argee to all the suggestions, especially to keeping the name of the function that checks ABI. After all its name reflects 99% of its functionality and it is idiomatic for 'check' functions to have side effects in this project.