* [Qemu-devel] [PATCH] mmap-alloc: check before use for ptr pointer
@ 2016-10-12 2:05 Gonglei
2016-10-12 6:45 ` Michael Tokarev
0 siblings, 1 reply; 6+ messages in thread
From: Gonglei @ 2016-10-12 2:05 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-trivial, herongguang.he, mjt, Gonglei
If ptr mmap failed, we don't need to do a superfluous
calculation for offset variable by ptr (MAP_FAILED).
Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
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 */
--
1.8.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] mmap-alloc: check before use for ptr pointer
2016-10-12 2:05 [Qemu-devel] [PATCH] mmap-alloc: check before use for ptr pointer Gonglei
@ 2016-10-12 6:45 ` Michael Tokarev
2016-10-12 7:37 ` Gonglei (Arei)
0 siblings, 1 reply; 6+ messages in thread
From: Michael Tokarev @ 2016-10-12 6:45 UTC (permalink / raw)
To: Gonglei, qemu-devel; +Cc: qemu-trivial, herongguang.he
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 <arei.gonglei@huawei.com>
> ---
> 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 */
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] mmap-alloc: check before use for ptr pointer
2016-10-12 6:45 ` Michael Tokarev
@ 2016-10-12 7:37 ` Gonglei (Arei)
2016-10-12 7:40 ` Paolo Bonzini
0 siblings, 1 reply; 6+ messages in thread
From: Gonglei (Arei) @ 2016-10-12 7:37 UTC (permalink / raw)
To: Michael Tokarev, qemu-devel; +Cc: qemu-trivial, Herongguang (Stephen)
> -----Original Message-----
> From: Michael Tokarev [mailto:mjt@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 <arei.gonglei@huawei.com>
> > ---
> > 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 */
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] mmap-alloc: check before use for ptr pointer
2016-10-12 7:37 ` Gonglei (Arei)
@ 2016-10-12 7:40 ` Paolo Bonzini
2016-10-12 7:54 ` Gonglei (Arei)
0 siblings, 1 reply; 6+ messages in thread
From: Paolo Bonzini @ 2016-10-12 7:40 UTC (permalink / raw)
To: Gonglei (Arei), Michael Tokarev, qemu-devel
Cc: qemu-trivial, Herongguang (Stephen)
On 12/10/2016 09:37, Gonglei (Arei) wrote:
>> -----Original Message-----
>> From: Michael Tokarev [mailto:mjt@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 <arei.gonglei@huawei.com>
>>> ---
>>> 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 */
>>>
>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] mmap-alloc: check before use for ptr pointer
2016-10-12 7:40 ` Paolo Bonzini
@ 2016-10-12 7:54 ` Gonglei (Arei)
2016-10-12 8:19 ` Paolo Bonzini
0 siblings, 1 reply; 6+ messages in thread
From: Gonglei (Arei) @ 2016-10-12 7:54 UTC (permalink / raw)
To: Paolo Bonzini, Michael Tokarev, qemu-devel
Cc: qemu-trivial, Herongguang (Stephen)
> -----Original Message-----
> From: Paolo Bonzini [mailto:paolo.bonzini@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-trivial@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:mjt@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 <arei.gonglei@huawei.com>
> >>> ---
> >>> 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 */
> >>>
> >
> >
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] mmap-alloc: check before use for ptr pointer
2016-10-12 7:54 ` Gonglei (Arei)
@ 2016-10-12 8:19 ` Paolo Bonzini
0 siblings, 0 replies; 6+ messages in thread
From: Paolo Bonzini @ 2016-10-12 8:19 UTC (permalink / raw)
To: Gonglei (Arei), Michael Tokarev, qemu-devel
Cc: qemu-trivial, Herongguang (Stephen)
On 12/10/2016 09:54, Gonglei (Arei) wrote:
>
>> -----Original Message-----
>> From: Paolo Bonzini [mailto:paolo.bonzini@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-trivial@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:mjt@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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-10-12 8:19 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-10-12 2:05 [Qemu-devel] [PATCH] mmap-alloc: check before use for ptr pointer Gonglei
2016-10-12 6:45 ` Michael Tokarev
2016-10-12 7:37 ` Gonglei (Arei)
2016-10-12 7:40 ` Paolo Bonzini
2016-10-12 7:54 ` Gonglei (Arei)
2016-10-12 8:19 ` Paolo Bonzini
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.