Re: [Qemu-devel] [PATCH] mmap-alloc: check before use for ptr pointer
On 12/10/2016 09:54, Gonglei (Arei) wrote: > >> -Original Message- >> From: Paolo Bonzini [mailto:paolo.bonz...@gmail.com] On Behalf Of Paolo >> Bonzini >> Sent: Wednesday, October 12, 2016 3:41 PM >> To: Gonglei (Arei); Michael Tokarev; qemu-devel@nongnu.org >> Cc: qemu-triv...@nongnu.org; Herongguang (Stephen) >> Subject: Re: [PATCH] mmap-alloc: check before use for ptr pointer >> >> >> >> On 12/10/2016 09:37, Gonglei (Arei) wrote: -Original Message- From: Michael Tokarev [mailto:m...@tls.msk.ru] Sent: Wednesday, October 12, 2016 2:46 PM Subject: Re: [PATCH] mmap-alloc: check before use for ptr pointer 12.10.2016 05:05, Gonglei wrote: > If ptr mmap failed, we don't need to do a superfluous > calculation for offset variable by ptr (MAP_FAILED). What's the point? There's no problem in extra calculation if mmap failed, yes, but do we really care? As of it is now, it is more compact and readable, and works. I think. >>> >>> I just think it's more reasonable because the ptr is checked after >>> used. Why do we need a extra calculation? >> >> Another reasonable objection (that however your patch doesn't fix) is >> that align is being used before the assertion: >> >> assert(!(align & (align - 1))); >> > > Eh, align is used at the beginning of the qemu_ram_mmap() ;) Yes, but the assertion is there because align is passed to QEMU_ALIGN_UP. Paolo
Re: [Qemu-devel] [PATCH] mmap-alloc: check before use for ptr pointer
> -Original Message-
> From: Paolo Bonzini [mailto:paolo.bonz...@gmail.com] On Behalf Of Paolo
> Bonzini
> Sent: Wednesday, October 12, 2016 3:41 PM
> To: Gonglei (Arei); Michael Tokarev; qemu-devel@nongnu.org
> Cc: qemu-triv...@nongnu.org; Herongguang (Stephen)
> Subject: Re: [PATCH] mmap-alloc: check before use for ptr pointer
>
>
>
> On 12/10/2016 09:37, Gonglei (Arei) wrote:
> >> -Original Message-
> >> From: Michael Tokarev [mailto:m...@tls.msk.ru]
> >> Sent: Wednesday, October 12, 2016 2:46 PM
> >> Subject: Re: [PATCH] mmap-alloc: check before use for ptr pointer
> >>
> >> 12.10.2016 05:05, Gonglei wrote:
> >>> If ptr mmap failed, we don't need to do a superfluous
> >>> calculation for offset variable by ptr (MAP_FAILED).
> >>
> >> What's the point? There's no problem in extra calculation
> >> if mmap failed, yes, but do we really care? As of it is now,
> >> it is more compact and readable, and works. I think.
> >>
> >
> > I just think it's more reasonable because the ptr is checked after
> > used. Why do we need a extra calculation?
>
> Another reasonable objection (that however your patch doesn't fix) is
> that align is being used before the assertion:
>
> assert(!(align & (align - 1)));
>
Eh, align is used at the beginning of the qemu_ram_mmap() ;)
Regards,
-Gonglei
> Paolo
>
> >
> > Regards,
> > -Gonglei
> >
> >> Thanks,
> >>
> >> /mjt
> >>
> >>> Signed-off-by: Gonglei
> >>> ---
> >>> util/mmap-alloc.c | 4 +++-
> >>> 1 file changed, 3 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/util/mmap-alloc.c b/util/mmap-alloc.c
> >>> index 5a85aa3..577862b 100644
> >>> --- a/util/mmap-alloc.c
> >>> +++ b/util/mmap-alloc.c
> >>> @@ -61,13 +61,15 @@ void *qemu_ram_mmap(int fd, size_t size, size_t
> >> align, bool shared)
> >>> #else
> >>> void *ptr = mmap(0, total, PROT_NONE, MAP_ANONYMOUS |
> >> MAP_PRIVATE, -1, 0);
> >>> #endif
> >>> -size_t offset = QEMU_ALIGN_UP((uintptr_t)ptr, align) -
> >>> (uintptr_t)ptr;
> >>> +size_t offset;
> >>> void *ptr1;
> >>>
> >>> if (ptr == MAP_FAILED) {
> >>> return MAP_FAILED;
> >>> }
> >>>
> >>> +offset = QEMU_ALIGN_UP((uintptr_t)ptr, align) - (uintptr_t)ptr;
> >>> +
> >>> /* Make sure align is a power of 2 */
> >>> assert(!(align & (align - 1)));
> >>> /* Always align to host page size */
> >>>
> >
> >
> >
Re: [Qemu-devel] [PATCH] mmap-alloc: check before use for ptr pointer
On 12/10/2016 09:37, Gonglei (Arei) wrote:
>> -Original Message-
>> From: Michael Tokarev [mailto:m...@tls.msk.ru]
>> Sent: Wednesday, October 12, 2016 2:46 PM
>> Subject: Re: [PATCH] mmap-alloc: check before use for ptr pointer
>>
>> 12.10.2016 05:05, Gonglei wrote:
>>> If ptr mmap failed, we don't need to do a superfluous
>>> calculation for offset variable by ptr (MAP_FAILED).
>>
>> What's the point? There's no problem in extra calculation
>> if mmap failed, yes, but do we really care? As of it is now,
>> it is more compact and readable, and works. I think.
>>
>
> I just think it's more reasonable because the ptr is checked after
> used. Why do we need a extra calculation?
Another reasonable objection (that however your patch doesn't fix) is
that align is being used before the assertion:
assert(!(align & (align - 1)));
Paolo
>
> Regards,
> -Gonglei
>
>> Thanks,
>>
>> /mjt
>>
>>> Signed-off-by: Gonglei
>>> ---
>>> util/mmap-alloc.c | 4 +++-
>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/util/mmap-alloc.c b/util/mmap-alloc.c
>>> index 5a85aa3..577862b 100644
>>> --- a/util/mmap-alloc.c
>>> +++ b/util/mmap-alloc.c
>>> @@ -61,13 +61,15 @@ void *qemu_ram_mmap(int fd, size_t size, size_t
>> align, bool shared)
>>> #else
>>> void *ptr = mmap(0, total, PROT_NONE, MAP_ANONYMOUS |
>> MAP_PRIVATE, -1, 0);
>>> #endif
>>> -size_t offset = QEMU_ALIGN_UP((uintptr_t)ptr, align) - (uintptr_t)ptr;
>>> +size_t offset;
>>> void *ptr1;
>>>
>>> if (ptr == MAP_FAILED) {
>>> return MAP_FAILED;
>>> }
>>>
>>> +offset = QEMU_ALIGN_UP((uintptr_t)ptr, align) - (uintptr_t)ptr;
>>> +
>>> /* Make sure align is a power of 2 */
>>> assert(!(align & (align - 1)));
>>> /* Always align to host page size */
>>>
>
>
>
Re: [Qemu-devel] [PATCH] mmap-alloc: check before use for ptr pointer
> -Original Message-
> From: Michael Tokarev [mailto:m...@tls.msk.ru]
> Sent: Wednesday, October 12, 2016 2:46 PM
> Subject: Re: [PATCH] mmap-alloc: check before use for ptr pointer
>
> 12.10.2016 05:05, Gonglei wrote:
> > If ptr mmap failed, we don't need to do a superfluous
> > calculation for offset variable by ptr (MAP_FAILED).
>
> What's the point? There's no problem in extra calculation
> if mmap failed, yes, but do we really care? As of it is now,
> it is more compact and readable, and works. I think.
>
I just think it's more reasonable because the ptr is checked after
used. Why do we need a extra calculation?
Regards,
-Gonglei
> Thanks,
>
> /mjt
>
> > Signed-off-by: Gonglei
> > ---
> > util/mmap-alloc.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/util/mmap-alloc.c b/util/mmap-alloc.c
> > index 5a85aa3..577862b 100644
> > --- a/util/mmap-alloc.c
> > +++ b/util/mmap-alloc.c
> > @@ -61,13 +61,15 @@ void *qemu_ram_mmap(int fd, size_t size, size_t
> align, bool shared)
> > #else
> > void *ptr = mmap(0, total, PROT_NONE, MAP_ANONYMOUS |
> MAP_PRIVATE, -1, 0);
> > #endif
> > -size_t offset = QEMU_ALIGN_UP((uintptr_t)ptr, align) - (uintptr_t)ptr;
> > +size_t offset;
> > void *ptr1;
> >
> > if (ptr == MAP_FAILED) {
> > return MAP_FAILED;
> > }
> >
> > +offset = QEMU_ALIGN_UP((uintptr_t)ptr, align) - (uintptr_t)ptr;
> > +
> > /* Make sure align is a power of 2 */
> > assert(!(align & (align - 1)));
> > /* Always align to host page size */
> >
Re: [Qemu-devel] [PATCH] mmap-alloc: check before use for ptr pointer
12.10.2016 05:05, Gonglei wrote:
If ptr mmap failed, we don't need to do a superfluous
calculation for offset variable by ptr (MAP_FAILED).
What's the point? There's no problem in extra calculation
if mmap failed, yes, but do we really care? As of it is now,
it is more compact and readable, and works. I think.
Thanks,
/mjt
Signed-off-by: Gonglei
---
util/mmap-alloc.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/util/mmap-alloc.c b/util/mmap-alloc.c
index 5a85aa3..577862b 100644
--- a/util/mmap-alloc.c
+++ b/util/mmap-alloc.c
@@ -61,13 +61,15 @@ void *qemu_ram_mmap(int fd, size_t size, size_t align, bool
shared)
#else
void *ptr = mmap(0, total, PROT_NONE, MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
#endif
-size_t offset = QEMU_ALIGN_UP((uintptr_t)ptr, align) - (uintptr_t)ptr;
+size_t offset;
void *ptr1;
if (ptr == MAP_FAILED) {
return MAP_FAILED;
}
+offset = QEMU_ALIGN_UP((uintptr_t)ptr, align) - (uintptr_t)ptr;
+
/* Make sure align is a power of 2 */
assert(!(align & (align - 1)));
/* Always align to host page size */
