| Message ID | 1503562911-2776-6-git-send-email-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Aug 24, 2017 at 10:21:50AM +0200, Igor Mammedov wrote: > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > target/ppc/translate_init.c | 27 +++++++++++---------------- > 1 file changed, 11 insertions(+), 16 deletions(-) > > diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c > index f1a559d..ca9f1e3 100644 > --- a/target/ppc/translate_init.c > +++ b/target/ppc/translate_init.c > @@ -34,6 +34,7 @@ > #include "hw/ppc/ppc.h" > #include "mmu-book3s-v3.h" > #include "sysemu/qtest.h" > +#include "qemu/cutils.h" > > //#define PPC_DUMP_CPU > //#define PPC_DEBUG_SPR > @@ -10203,22 +10204,16 @@ static ObjectClass *ppc_cpu_class_by_name(const char *name) > char *cpu_model, *typename; > ObjectClass *oc; > const char *p; > - int i, len; > - > - /* Check if the given name is a PVR */ > - len = strlen(name); > - if (len == 10 && name[0] == '0' && name[1] == 'x') { > - p = name + 2; > - goto check_pvr; > - } else if (len == 8) { > - p = name; > - check_pvr: > - for (i = 0; i < 8; i++) { > - if (!qemu_isxdigit(*p++)) > - break; > - } > - if (i == 8) { > - return OBJECT_CLASS(ppc_cpu_class_by_pvr(strtoul(name, NULL, 16))); > + unsigned long pvr; > + > + /* Lookup by PVR if cpu_model is valid 8 digit hex number > + * (excl: 0x prefix if present) > + */ > + if (!qemu_strtoul(name, &p, 16, &pvr)) { > + int len = p - name; > + len = (len == 10) && (name[1] == 'x') ? len - 2 : len; > + if (len == 8) { > + return OBJECT_CLASS(ppc_cpu_class_by_pvr(pvr)); This doesn't look quite right. A hex string of a PVR followed by other stuff isn't a valid option, so if p (endptr) doesn't point to the end of the string, then we shouldn't be using this path (from the looks of qemu_strtoul() we can simply omit the endptr parameter to get that behaviour). I'm not sure what the messing around with len is for, either. If it's a valid hex string we can try this, I don't see that we need further checks. > } > } >
On Fri, 25 Aug 2017 14:16:44 +1000 David Gibson <david@gibson.dropbear.id.au> wrote: > On Thu, Aug 24, 2017 at 10:21:50AM +0200, Igor Mammedov wrote: > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > --- > > target/ppc/translate_init.c | 27 +++++++++++---------------- > > 1 file changed, 11 insertions(+), 16 deletions(-) > > > > diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c > > index f1a559d..ca9f1e3 100644 > > --- a/target/ppc/translate_init.c > > +++ b/target/ppc/translate_init.c > > @@ -34,6 +34,7 @@ > > #include "hw/ppc/ppc.h" > > #include "mmu-book3s-v3.h" > > #include "sysemu/qtest.h" > > +#include "qemu/cutils.h" > > > > //#define PPC_DUMP_CPU > > //#define PPC_DEBUG_SPR > > @@ -10203,22 +10204,16 @@ static ObjectClass *ppc_cpu_class_by_name(const char *name) > > char *cpu_model, *typename; > > ObjectClass *oc; > > const char *p; > > - int i, len; > > - > > - /* Check if the given name is a PVR */ > > - len = strlen(name); > > - if (len == 10 && name[0] == '0' && name[1] == 'x') { > > - p = name + 2; > > - goto check_pvr; > > - } else if (len == 8) { > > - p = name; > > - check_pvr: > > - for (i = 0; i < 8; i++) { > > - if (!qemu_isxdigit(*p++)) > > - break; > > - } > > - if (i == 8) { > > - return OBJECT_CLASS(ppc_cpu_class_by_pvr(strtoul(name, NULL, 16))); > > + unsigned long pvr; > > + > > + /* Lookup by PVR if cpu_model is valid 8 digit hex number > > + * (excl: 0x prefix if present) > > + */ > > + if (!qemu_strtoul(name, &p, 16, &pvr)) { > > + int len = p - name; > > + len = (len == 10) && (name[1] == 'x') ? len - 2 : len; > > + if (len == 8) { > > + return OBJECT_CLASS(ppc_cpu_class_by_pvr(pvr)); > > This doesn't look quite right. A hex string of a PVR followed by > other stuff isn't a valid option, so if p (endptr) doesn't point to > the end of the string, then we shouldn't be using this path yep, my mistake, I've lost somewhere *p == '\0' check when cleaning up the patch >(from the > looks of qemu_strtoul() we can simply omit the endptr parameter to get > that behaviour). I'm not sure what the messing around with len is > for, either. If it's a valid hex string we can try this, I don't see > that we need further checks. I've tried to keep current limitation here i.e. 8 hex symbols, but if any hex number is fine then doing qemu_strtoul(name, NULL, 16, &pvr) is even better, so would you prefer to drop 8 symbol check altogether? > > > } > > } > > >
On Fri, Aug 25, 2017 at 09:40:37AM +0200, Igor Mammedov wrote: > On Fri, 25 Aug 2017 14:16:44 +1000 > David Gibson <david@gibson.dropbear.id.au> wrote: > > > On Thu, Aug 24, 2017 at 10:21:50AM +0200, Igor Mammedov wrote: > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > --- > > > target/ppc/translate_init.c | 27 +++++++++++---------------- > > > 1 file changed, 11 insertions(+), 16 deletions(-) > > > > > > diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c > > > index f1a559d..ca9f1e3 100644 > > > --- a/target/ppc/translate_init.c > > > +++ b/target/ppc/translate_init.c > > > @@ -34,6 +34,7 @@ > > > #include "hw/ppc/ppc.h" > > > #include "mmu-book3s-v3.h" > > > #include "sysemu/qtest.h" > > > +#include "qemu/cutils.h" > > > > > > //#define PPC_DUMP_CPU > > > //#define PPC_DEBUG_SPR > > > @@ -10203,22 +10204,16 @@ static ObjectClass *ppc_cpu_class_by_name(const char *name) > > > char *cpu_model, *typename; > > > ObjectClass *oc; > > > const char *p; > > > - int i, len; > > > - > > > - /* Check if the given name is a PVR */ > > > - len = strlen(name); > > > - if (len == 10 && name[0] == '0' && name[1] == 'x') { > > > - p = name + 2; > > > - goto check_pvr; > > > - } else if (len == 8) { > > > - p = name; > > > - check_pvr: > > > - for (i = 0; i < 8; i++) { > > > - if (!qemu_isxdigit(*p++)) > > > - break; > > > - } > > > - if (i == 8) { > > > - return OBJECT_CLASS(ppc_cpu_class_by_pvr(strtoul(name, NULL, 16))); > > > + unsigned long pvr; > > > + > > > + /* Lookup by PVR if cpu_model is valid 8 digit hex number > > > + * (excl: 0x prefix if present) > > > + */ > > > + if (!qemu_strtoul(name, &p, 16, &pvr)) { > > > + int len = p - name; > > > + len = (len == 10) && (name[1] == 'x') ? len - 2 : len; > > > + if (len == 8) { > > > + return OBJECT_CLASS(ppc_cpu_class_by_pvr(pvr)); > > > > This doesn't look quite right. A hex string of a PVR followed by > > other stuff isn't a valid option, so if p (endptr) doesn't point to > > the end of the string, then we shouldn't be using this path > yep, my mistake, I've lost somewhere *p == '\0' check when cleaning up the patch Ok. > >(from the > > looks of qemu_strtoul() we can simply omit the endptr parameter to get > > that behaviour). I'm not sure what the messing around with len is > > for, either. If it's a valid hex string we can try this, I don't see > > that we need further checks. > I've tried to keep current limitation here i.e. 8 hex symbols, > but if any hex number is fine then doing > qemu_strtoul(name, NULL, 16, &pvr) > is even better, so would you prefer to drop 8 symbol check altogether? Yeah, I don't see any value to the 8 character check.
On Fri, 25 Aug 2017 19:32:29 +1000 David Gibson <david@gibson.dropbear.id.au> wrote: > On Fri, Aug 25, 2017 at 09:40:37AM +0200, Igor Mammedov wrote: > > On Fri, 25 Aug 2017 14:16:44 +1000 > > David Gibson <david@gibson.dropbear.id.au> wrote: > > > > > On Thu, Aug 24, 2017 at 10:21:50AM +0200, Igor Mammedov wrote: > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > --- > > > > target/ppc/translate_init.c | 27 +++++++++++---------------- > > > > 1 file changed, 11 insertions(+), 16 deletions(-) > > > > > > > > diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c > > > > index f1a559d..ca9f1e3 100644 > > > > --- a/target/ppc/translate_init.c > > > > +++ b/target/ppc/translate_init.c > > > > @@ -34,6 +34,7 @@ > > > > #include "hw/ppc/ppc.h" > > > > #include "mmu-book3s-v3.h" > > > > #include "sysemu/qtest.h" > > > > +#include "qemu/cutils.h" > > > > > > > > //#define PPC_DUMP_CPU > > > > //#define PPC_DEBUG_SPR > > > > @@ -10203,22 +10204,16 @@ static ObjectClass *ppc_cpu_class_by_name(const char *name) > > > > char *cpu_model, *typename; > > > > ObjectClass *oc; > > > > const char *p; > > > > - int i, len; > > > > - > > > > - /* Check if the given name is a PVR */ > > > > - len = strlen(name); > > > > - if (len == 10 && name[0] == '0' && name[1] == 'x') { > > > > - p = name + 2; > > > > - goto check_pvr; > > > > - } else if (len == 8) { > > > > - p = name; > > > > - check_pvr: > > > > - for (i = 0; i < 8; i++) { > > > > - if (!qemu_isxdigit(*p++)) > > > > - break; > > > > - } > > > > - if (i == 8) { > > > > - return OBJECT_CLASS(ppc_cpu_class_by_pvr(strtoul(name, NULL, 16))); > > > > + unsigned long pvr; > > > > + > > > > + /* Lookup by PVR if cpu_model is valid 8 digit hex number > > > > + * (excl: 0x prefix if present) > > > > + */ > > > > + if (!qemu_strtoul(name, &p, 16, &pvr)) { > > > > + int len = p - name; > > > > + len = (len == 10) && (name[1] == 'x') ? len - 2 : len; > > > > + if (len == 8) { > > > > + return OBJECT_CLASS(ppc_cpu_class_by_pvr(pvr)); > > > > > > This doesn't look quite right. A hex string of a PVR followed by > > > other stuff isn't a valid option, so if p (endptr) doesn't point to > > > the end of the string, then we shouldn't be using this path > > yep, my mistake, I've lost somewhere *p == '\0' check when cleaning up the patch > > Ok. > > > >(from the > > > looks of qemu_strtoul() we can simply omit the endptr parameter to get > > > that behaviour). I'm not sure what the messing around with len is > > > for, either. If it's a valid hex string we can try this, I don't see > > > that we need further checks. > > I've tried to keep current limitation here i.e. 8 hex symbols, > > but if any hex number is fine then doing > > qemu_strtoul(name, NULL, 16, &pvr) > > is even better, so would you prefer to drop 8 symbol check altogether? > > Yeah, I don't see any value to the 8 character check. then I'll drop it and do as you've suggested.
On Fri, 25 Aug 2017 19:32:29 +1000 David Gibson <david@gibson.dropbear.id.au> wrote: > On Fri, Aug 25, 2017 at 09:40:37AM +0200, Igor Mammedov wrote: > > On Fri, 25 Aug 2017 14:16:44 +1000 > > David Gibson <david@gibson.dropbear.id.au> wrote: > > > > > On Thu, Aug 24, 2017 at 10:21:50AM +0200, Igor Mammedov wrote: > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > --- > > > > target/ppc/translate_init.c | 27 +++++++++++---------------- > > > > 1 file changed, 11 insertions(+), 16 deletions(-) > > > > > > > > diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c > > > > index f1a559d..ca9f1e3 100644 > > > > --- a/target/ppc/translate_init.c > > > > +++ b/target/ppc/translate_init.c > > > > @@ -34,6 +34,7 @@ > > > > #include "hw/ppc/ppc.h" > > > > #include "mmu-book3s-v3.h" > > > > #include "sysemu/qtest.h" > > > > +#include "qemu/cutils.h" > > > > > > > > //#define PPC_DUMP_CPU > > > > //#define PPC_DEBUG_SPR > > > > @@ -10203,22 +10204,16 @@ static ObjectClass *ppc_cpu_class_by_name(const char *name) > > > > char *cpu_model, *typename; > > > > ObjectClass *oc; > > > > const char *p; > > > > - int i, len; > > > > - > > > > - /* Check if the given name is a PVR */ > > > > - len = strlen(name); > > > > - if (len == 10 && name[0] == '0' && name[1] == 'x') { > > > > - p = name + 2; > > > > - goto check_pvr; > > > > - } else if (len == 8) { > > > > - p = name; > > > > - check_pvr: > > > > - for (i = 0; i < 8; i++) { > > > > - if (!qemu_isxdigit(*p++)) > > > > - break; > > > > - } > > > > - if (i == 8) { > > > > - return OBJECT_CLASS(ppc_cpu_class_by_pvr(strtoul(name, NULL, 16))); > > > > + unsigned long pvr; > > > > + > > > > + /* Lookup by PVR if cpu_model is valid 8 digit hex number > > > > + * (excl: 0x prefix if present) > > > > + */ > > > > + if (!qemu_strtoul(name, &p, 16, &pvr)) { > > > > + int len = p - name; > > > > + len = (len == 10) && (name[1] == 'x') ? len - 2 : len; > > > > + if (len == 8) { > > > > + return OBJECT_CLASS(ppc_cpu_class_by_pvr(pvr)); > > > > > > This doesn't look quite right. A hex string of a PVR followed by > > > other stuff isn't a valid option, so if p (endptr) doesn't point to > > > the end of the string, then we shouldn't be using this path > > yep, my mistake, I've lost somewhere *p == '\0' check when cleaning up the patch > > Ok. > > > >(from the > > > looks of qemu_strtoul() we can simply omit the endptr parameter to get > > > that behaviour). I'm not sure what the messing around with len is > > > for, either. If it's a valid hex string we can try this, I don't see > > > that we need further checks. > > I've tried to keep current limitation here i.e. 8 hex symbols, > > but if any hex number is fine then doing > > qemu_strtoul(name, NULL, 16, &pvr) > > is even better, so would you prefer to drop 8 symbol check altogether? > > Yeah, I don't see any value to the 8 character check. there are cpu models consisting of pure numbers => valid hex number so if check is dropped then it lookup will go PVR route, that will fail there. So check should be there to avoid regression. I'll fix whole string consumption check that I've missed before and respin with it.
On Wed, Aug 30, 2017 at 12:50:33PM +0200, Igor Mammedov wrote: > On Fri, 25 Aug 2017 19:32:29 +1000 > David Gibson <david@gibson.dropbear.id.au> wrote: > > > On Fri, Aug 25, 2017 at 09:40:37AM +0200, Igor Mammedov wrote: > > > On Fri, 25 Aug 2017 14:16:44 +1000 > > > David Gibson <david@gibson.dropbear.id.au> wrote: > > > > > > > On Thu, Aug 24, 2017 at 10:21:50AM +0200, Igor Mammedov wrote: > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > > --- > > > > > target/ppc/translate_init.c | 27 +++++++++++---------------- > > > > > 1 file changed, 11 insertions(+), 16 deletions(-) > > > > > > > > > > diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c > > > > > index f1a559d..ca9f1e3 100644 > > > > > --- a/target/ppc/translate_init.c > > > > > +++ b/target/ppc/translate_init.c > > > > > @@ -34,6 +34,7 @@ > > > > > #include "hw/ppc/ppc.h" > > > > > #include "mmu-book3s-v3.h" > > > > > #include "sysemu/qtest.h" > > > > > +#include "qemu/cutils.h" > > > > > > > > > > //#define PPC_DUMP_CPU > > > > > //#define PPC_DEBUG_SPR > > > > > @@ -10203,22 +10204,16 @@ static ObjectClass *ppc_cpu_class_by_name(const char *name) > > > > > char *cpu_model, *typename; > > > > > ObjectClass *oc; > > > > > const char *p; > > > > > - int i, len; > > > > > - > > > > > - /* Check if the given name is a PVR */ > > > > > - len = strlen(name); > > > > > - if (len == 10 && name[0] == '0' && name[1] == 'x') { > > > > > - p = name + 2; > > > > > - goto check_pvr; > > > > > - } else if (len == 8) { > > > > > - p = name; > > > > > - check_pvr: > > > > > - for (i = 0; i < 8; i++) { > > > > > - if (!qemu_isxdigit(*p++)) > > > > > - break; > > > > > - } > > > > > - if (i == 8) { > > > > > - return OBJECT_CLASS(ppc_cpu_class_by_pvr(strtoul(name, NULL, 16))); > > > > > + unsigned long pvr; > > > > > + > > > > > + /* Lookup by PVR if cpu_model is valid 8 digit hex number > > > > > + * (excl: 0x prefix if present) > > > > > + */ > > > > > + if (!qemu_strtoul(name, &p, 16, &pvr)) { > > > > > + int len = p - name; > > > > > + len = (len == 10) && (name[1] == 'x') ? len - 2 : len; > > > > > + if (len == 8) { > > > > > + return OBJECT_CLASS(ppc_cpu_class_by_pvr(pvr)); > > > > > > > > This doesn't look quite right. A hex string of a PVR followed by > > > > other stuff isn't a valid option, so if p (endptr) doesn't point to > > > > the end of the string, then we shouldn't be using this path > > > yep, my mistake, I've lost somewhere *p == '\0' check when cleaning up the patch > > > > Ok. > > > > > >(from the > > > > looks of qemu_strtoul() we can simply omit the endptr parameter to get > > > > that behaviour). I'm not sure what the messing around with len is > > > > for, either. If it's a valid hex string we can try this, I don't see > > > > that we need further checks. > > > I've tried to keep current limitation here i.e. 8 hex symbols, > > > but if any hex number is fine then doing > > > qemu_strtoul(name, NULL, 16, &pvr) > > > is even better, so would you prefer to drop 8 symbol check altogether? > > > > Yeah, I don't see any value to the 8 character check. > there are cpu models consisting of pure numbers => valid hex number > so if check is dropped then it lookup will go PVR route, > that will fail there. > So check should be there to avoid regression. Ah. Good point. > I'll fix whole string consumption check that I've missed before > and respin with it. > > >
diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c index f1a559d..ca9f1e3 100644 --- a/target/ppc/translate_init.c +++ b/target/ppc/translate_init.c @@ -34,6 +34,7 @@ #include "hw/ppc/ppc.h" #include "mmu-book3s-v3.h" #include "sysemu/qtest.h" +#include "qemu/cutils.h" //#define PPC_DUMP_CPU //#define PPC_DEBUG_SPR @@ -10203,22 +10204,16 @@ static ObjectClass *ppc_cpu_class_by_name(const char *name) char *cpu_model, *typename; ObjectClass *oc; const char *p; - int i, len; - - /* Check if the given name is a PVR */ - len = strlen(name); - if (len == 10 && name[0] == '0' && name[1] == 'x') { - p = name + 2; - goto check_pvr; - } else if (len == 8) { - p = name; - check_pvr: - for (i = 0; i < 8; i++) { - if (!qemu_isxdigit(*p++)) - break; - } - if (i == 8) { - return OBJECT_CLASS(ppc_cpu_class_by_pvr(strtoul(name, NULL, 16))); + unsigned long pvr; + + /* Lookup by PVR if cpu_model is valid 8 digit hex number + * (excl: 0x prefix if present) + */ + if (!qemu_strtoul(name, &p, 16, &pvr)) { + int len = p - name; + len = (len == 10) && (name[1] == 'x') ? len - 2 : len; + if (len == 8) { + return OBJECT_CLASS(ppc_cpu_class_by_pvr(pvr)); } }
Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- target/ppc/translate_init.c | 27 +++++++++++---------------- 1 file changed, 11 insertions(+), 16 deletions(-)