On Fri, Feb 20, 2015 at 08:16:17 -0500, John Ferlan wrote:
> On 02/18/2015 09:16 AM, Peter Krempa wrote:
> > Commit 60f7303c151cccdbe214b9f9ac59ecaf95cbf24b introduced the error
> > message but it's unclear whether the persistent config or the live
> > config tripped the message. Later the LXC driver copied the same code.
> >
> > Separate the message which will also clarify the code.
> > ---
> > src/lxc/lxc_driver.c | 21 +++++++++++----------
> > src/qemu/qemu_driver.c | 19 ++++++++++---------
> > 2 files changed, 21 insertions(+), 19 deletions(-)
> >
> > diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
> > index 3adb21d..5af0554 100644
> > --- a/src/lxc/lxc_driver.c
> > +++ b/src/lxc/lxc_driver.c
> > @@ -742,18 +742,19 @@ static int lxcDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem,
> > goto cleanup;
> > }
> > } else {
> > - unsigned long oldmax = 0;
> > -
> > - if (flags & VIR_DOMAIN_AFFECT_LIVE)
> > - oldmax = vm->def->mem.max_balloon;
> > - if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
> > - if (!oldmax || oldmax > persistentDef->mem.max_balloon)
> > - oldmax = persistentDef->mem.max_balloon;
> > + if (flags & VIR_DOMAIN_AFFECT_LIVE &&
> > + newmem > vm->def->mem.max_balloon) {
> > + virReportError(VIR_ERR_INVALID_ARG, "%s",
> > + _("max memory of the live instance can't be less "
> > + "than the current memory"));
> > + goto cleanup;
> > }
> >
> > - if (newmem > oldmax) {
> > - virReportError(VIR_ERR_INVALID_ARG,
> > - "%s", _("Cannot set memory higher than max memory"));
> > + if (flags & VIR_DOMAIN_AFFECT_CONFIG &&
> > + newmem > persistentDef->mem.max_balloon) {
> > + virReportError(VIR_ERR_INVALID_ARG, "%s",
> > + _("max memory of the persistent configuration can't "
> > + "be less than the current memory"));
>
> This reads awkwardly... This is the non maxmem path - that is we're just
> setting memory and the comparison is that if the new memory value is
> greater than the defined maximum, then there is an error.
>
> Could be done in one (somewhat ugly) statement too...
I specifically wanted to avoid one statement ...
>
> if ((flags & VIR_DOMAIN_AFFECT_LIVE &&
> newmem > vm->def->mem.max_balloon) ||
> (flags & VIR_DOMAIN_AFFECT_CONFIG &&
> newmem > persistentDef->mem.max_balloon)) {
> unsigned long maxmem = flags & VIR_DOMAIN_AFFECT_LIVE ?
> vm->def->mem.max_balloon :
> persistentDef->mem.max_balloon;a
... as you need an intermediate variable ...
> virReportError(VIR_ERR_INVALID_ARG,
> _("new memory value '%lu' cannot be greater "
> "than the maximum value '%lu' for the %s"),
> newmem, maxmem, flags & VIR_DOMAIN_AFFECT_LIVE ?
... ternary operators ...
> _("live instance") :
> _("persistent configuration"));
... and hard to translate strings.
>
> ACK on the idea and I'm sure you'll be able to do the message properly.
> I did try to do a "cond ? arg2, arg3 : arg2, arg3" instead of setting
> maxmem, but wasn't successful in a quick attempt...
I'll try to improve the error message but I will not change the
two-condition approach.
Peter
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list