On Thu, Sep 25, 2014 at 12:23:31PM +0800, Jincheng Miao wrote: > When detected invalid 'memAccess', virCPUDefParseXML should report > error. > > Resolves https://bugzilla.redhat.com/show_bug.cgi?id=1146334 > > Signed-off-by: Jincheng Miao <jmiao(a)redhat.com&gt; > --- > src/conf/cpu_conf.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c > index 116aa58..a1081b9 100644 > --- a/src/conf/cpu_conf.c > +++ b/src/conf/cpu_conf.c > @@ -510,13 +510,13 @@ virCPUDefParseXML(xmlNodePtr node, > def->cells[cur_cell].memAccess = > virMemAccessTypeFromString(memAccessStr); > > - if (def->cells[cur_cell].memAccess <= 0) { > + if ((int)def->cells[cur_cell].memAccess <= 0) { Any reason for the explicit cast in here?

> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > _("Invalid 'memAccess' attribute " > "value '%s'"), > memAccessStr); > VIR_FREE(memAccessStr); > - goto cleanup; > + goto error; ACK to this line. Martin

On 09/25/14 09:25, Jincheng Miao wrote: > On 09/25/2014 01:48 PM, Martin Kletzander wrote: >> On Thu, Sep 25, 2014 at 12:23:31PM +0800, Jincheng Miao wrote: >>> When detected invalid 'memAccess', virCPUDefParseXML should report >>> error. >>> >>> Resolves https://bugzilla.redhat.com/show_bug.cgi?id=1146334 >>> >>> Signed-off-by: Jincheng Miao <jmiao(a)redhat.com&gt; >>> --- >>> src/conf/cpu_conf.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c >>> index 116aa58..a1081b9 100644 >>> --- a/src/conf/cpu_conf.c >>> +++ b/src/conf/cpu_conf.c >>> @@ -510,13 +510,13 @@ virCPUDefParseXML(xmlNodePtr node, >>> def->cells[cur_cell].memAccess = >>> virMemAccessTypeFromString(memAccessStr); >>> >>> - if (def->cells[cur_cell].memAccess <= 0) { >>> + if ((int)def->cells[cur_cell].memAccess <= 0) { >> Any reason for the explicit cast in here? > Yes, I found enum virMemAccess will be convert to uint, like: > > (gdb) c > Continuing. > > Breakpoint 2, virCPUDefParseXML (node=<optimized out>, > ctxt=ctxt@entry=0x7f92e80034a0, mode=mode@entry=VIR_CPU_TYPE_GUEST) > at conf/cpu_conf.c:513 > 513 if (def->cells[cur_cell].memAccess <= 0) { > (gdb) whatis def->cells[cur_cell].memAccess > type = virMemAccess > (gdb) p def->cells[cur_cell].memAccess > $1 = 4294967295 > (gdb) p def->cells[cur_cell].memAccess <= 0 > $2 = 0 > > In that case, use an intermediate variable with the correct type and assign it only after you verify that the value is correct.

Peter