| Message ID | 20180829153624.12299-2-david@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | memory-device: complete refactoring + virtio-pmem | expand |
* David Hildenbrand (david@redhat.com) wrote: > The "at" should actually be a "before". > if (new_addr < address_space_start) > -> "can't add memory ... before... $address_space_start" > > So it looks similar to the other check > } else if ((new_addr + size) > address_space_end) > -> "can't add memory ... beyond..." > > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > hw/mem/memory-device.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c > index 6de4f70bb4..efacbc2a7d 100644 > --- a/hw/mem/memory-device.c > +++ b/hw/mem/memory-device.c > @@ -146,7 +146,8 @@ uint64_t memory_device_get_free_addr(MachineState *ms, const uint64_t *hint, > new_addr = *hint; > if (new_addr < address_space_start) { > error_setg(errp, "can't add memory [0x%" PRIx64 ":0x%" PRIx64 > - "] at 0x%" PRIx64, new_addr, size, address_space_start); > + "] before 0x%" PRIx64, new_addr, size, > + address_space_start); OK, so Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > return 0; > } else if ((new_addr + size) > address_space_end) { That comparison (that's already there) doesn't look wrap-safe? Dave > error_setg(errp, "can't add memory [0x%" PRIx64 ":0x%" PRIx64 > -- > 2.17.1 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Wed, 29 Aug 2018 17:36:05 +0200 David Hildenbrand <david@redhat.com> wrote: > The "at" should actually be a "before". > if (new_addr < address_space_start) > -> "can't add memory ... before... $address_space_start" > > So it looks similar to the other check > } else if ((new_addr + size) > address_space_end) > -> "can't add memory ... beyond..." > > Signed-off-by: David Hildenbrand <david@redhat.com> Reviewed-by: Igor Mammedov <imammedo@redhat.com> > --- > hw/mem/memory-device.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c > index 6de4f70bb4..efacbc2a7d 100644 > --- a/hw/mem/memory-device.c > +++ b/hw/mem/memory-device.c > @@ -146,7 +146,8 @@ uint64_t memory_device_get_free_addr(MachineState *ms, const uint64_t *hint, > new_addr = *hint; > if (new_addr < address_space_start) { > error_setg(errp, "can't add memory [0x%" PRIx64 ":0x%" PRIx64 > - "] at 0x%" PRIx64, new_addr, size, address_space_start); > + "] before 0x%" PRIx64, new_addr, size, > + address_space_start); > return 0; > } else if ((new_addr + size) > address_space_end) { > error_setg(errp, "can't add memory [0x%" PRIx64 ":0x%" PRIx64
Am 30.08.18 um 10:53 schrieb Dr. David Alan Gilbert: > * David Hildenbrand (david@redhat.com) wrote: >> The "at" should actually be a "before". >> if (new_addr < address_space_start) >> -> "can't add memory ... before... $address_space_start" >> >> So it looks similar to the other check >> } else if ((new_addr + size) > address_space_end) >> -> "can't add memory ... beyond..." >> >> Signed-off-by: David Hildenbrand <david@redhat.com> >> --- >> hw/mem/memory-device.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c >> index 6de4f70bb4..efacbc2a7d 100644 >> --- a/hw/mem/memory-device.c >> +++ b/hw/mem/memory-device.c >> @@ -146,7 +146,8 @@ uint64_t memory_device_get_free_addr(MachineState *ms, const uint64_t *hint, >> new_addr = *hint; >> if (new_addr < address_space_start) { >> error_setg(errp, "can't add memory [0x%" PRIx64 ":0x%" PRIx64 >> - "] at 0x%" PRIx64, new_addr, size, address_space_start); >> + "] before 0x%" PRIx64, new_addr, size, >> + address_space_start); > > OK, so > > > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > >> return 0; >> } else if ((new_addr + size) > address_space_end) { > > That comparison (that's already there) doesn't look wrap-safe? > Indeed, but it is not the only one in this file. I'll add a patch for these. Thanks!
diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c index 6de4f70bb4..efacbc2a7d 100644 --- a/hw/mem/memory-device.c +++ b/hw/mem/memory-device.c @@ -146,7 +146,8 @@ uint64_t memory_device_get_free_addr(MachineState *ms, const uint64_t *hint, new_addr = *hint; if (new_addr < address_space_start) { error_setg(errp, "can't add memory [0x%" PRIx64 ":0x%" PRIx64 - "] at 0x%" PRIx64, new_addr, size, address_space_start); + "] before 0x%" PRIx64, new_addr, size, + address_space_start); return 0; } else if ((new_addr + size) > address_space_end) { error_setg(errp, "can't add memory [0x%" PRIx64 ":0x%" PRIx64
The "at" should actually be a "before". if (new_addr < address_space_start) -> "can't add memory ... before... $address_space_start" So it looks similar to the other check } else if ((new_addr + size) > address_space_end) -> "can't add memory ... beyond..." Signed-off-by: David Hildenbrand <david@redhat.com> --- hw/mem/memory-device.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)