[PATCH v2] util/memfd: allow allocating 0 bytes

[PATCH v2] util/memfd: allow allocating 0 bytes

[Elisha Hollander]

[-- Attachment #1: Type: text/plain, Size: 994 bytes --]

Sorry for former patch something is messed up with my email.

Signed-off-by: donno2048 <just4now666666@gmail.com>
---
 util/memfd.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/util/memfd.c b/util/memfd.c
index 8a2e906..e96e5af 100644
--- a/util/memfd.c
+++ b/util/memfd.c
@@ -108,7 +108,7 @@ err:
 void *qemu_memfd_alloc(const char *name, size_t size, unsigned int seals,
                        int *fd, Error **errp)
 {
- void *ptr;
+ void *ptr = NULL;
     int mfd = qemu_memfd_create(name, size, false, 0, seals, NULL);

     /* some systems have memfd without sealing */
@@ -131,9 +131,11 @@ void *qemu_memfd_alloc(const char *name, size_t size,
unsigned int seals,
         }
     }

- ptr = mmap(0, size, PROT_READ | PROT_WRITE, MAP_SHARED, mfd, 0);
- if (ptr == MAP_FAILED) {
- goto err;
+ if (size != 0) {
+ ptr = mmap(0, size, PROT_READ | PROT_WRITE, MAP_SHARED, mfd, 0);
+ if (ptr == MAP_FAILED) {
+ goto err;
+ }
     }

     *fd = mfd;
--
2.30.2

[-- Attachment #2: Type: text/html, Size: 1935 bytes --]

Re: [PATCH v2] util/memfd: allow allocating 0 bytes

[Daniel P. Berrangé]

On Tue, May 06, 2025 at 07:17:25PM +0300, Elisha Hollander wrote:
> Sorry for former patch something is messed up with my email.

The commit message needs to explain what problem is being solved by
making this change as allowing 0 bytes looks dubious on the surface.

> 
> Signed-off-by: donno2048 <just4now666666@gmail.com>
> ---
>  util/memfd.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/util/memfd.c b/util/memfd.c
> index 8a2e906..e96e5af 100644
> --- a/util/memfd.c
> +++ b/util/memfd.c
> @@ -108,7 +108,7 @@ err:
>  void *qemu_memfd_alloc(const char *name, size_t size, unsigned int seals,
>                         int *fd, Error **errp)
>  {
> - void *ptr;
> + void *ptr = NULL;
>      int mfd = qemu_memfd_create(name, size, false, 0, seals, NULL);
> 
>      /* some systems have memfd without sealing */
> @@ -131,9 +131,11 @@ void *qemu_memfd_alloc(const char *name, size_t size,
> unsigned int seals,
>          }
>      }
> 
> - ptr = mmap(0, size, PROT_READ | PROT_WRITE, MAP_SHARED, mfd, 0);
> - if (ptr == MAP_FAILED) {
> - goto err;
> + if (size != 0) {
> + ptr = mmap(0, size, PROT_READ | PROT_WRITE, MAP_SHARED, mfd, 0);
> + if (ptr == MAP_FAILED) {
> + goto err;
> + }
>      }

This patch is mangled.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2] util/memfd: allow allocating 0 bytes

[Elisha Hollander]

[-- Attachment #1: Type: text/plain, Size: 2312 bytes --]

Gave an example for a case where QEMU would try to allocate 0 bytes thus
fail here in the original version of the patch.

> As I mentioned earlier, let's say you don't initialize the vertical
display end registers, and set the minimum scanline register, the emulation
will then have to allocate some display buffer, but because the vertical
display end is initilized as 0 the buffer will be empty and the program
break.

I have no idea as for why my emails are getting messed up... :/

Have to go now, will try and send it again tomorrow probably...

On Tue, May 6, 2025, 19:37 Daniel P. Berrangé <berrange@redhat.com> wrote:

> On Tue, May 06, 2025 at 07:17:25PM +0300, Elisha Hollander wrote:
> > Sorry for former patch something is messed up with my email.
>
> The commit message needs to explain what problem is being solved by
> making this change as allowing 0 bytes looks dubious on the surface.
>
> >
> > Signed-off-by: donno2048 <just4now666666@gmail.com>
> > ---
> >  util/memfd.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/util/memfd.c b/util/memfd.c
> > index 8a2e906..e96e5af 100644
> > --- a/util/memfd.c
> > +++ b/util/memfd.c
> > @@ -108,7 +108,7 @@ err:
> >  void *qemu_memfd_alloc(const char *name, size_t size, unsigned int
> seals,
> >                         int *fd, Error **errp)
> >  {
> > - void *ptr;
> > + void *ptr = NULL;
> >      int mfd = qemu_memfd_create(name, size, false, 0, seals, NULL);
> >
> >      /* some systems have memfd without sealing */
> > @@ -131,9 +131,11 @@ void *qemu_memfd_alloc(const char *name, size_t
> size,
> > unsigned int seals,
> >          }
> >      }
> >
> > - ptr = mmap(0, size, PROT_READ | PROT_WRITE, MAP_SHARED, mfd, 0);
> > - if (ptr == MAP_FAILED) {
> > - goto err;
> > + if (size != 0) {
> > + ptr = mmap(0, size, PROT_READ | PROT_WRITE, MAP_SHARED, mfd, 0);
> > + if (ptr == MAP_FAILED) {
> > + goto err;
> > + }
> >      }
>
> This patch is mangled.
>
>
> With regards,
> Daniel
> --
> |: https://berrange.com      -o-
> https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-
> https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-
> https://www.instagram.com/dberrange :|
>
>

[-- Attachment #2: Type: text/html, Size: 3616 bytes --]

Re: [PATCH v2] util/memfd: allow allocating 0 bytes

[Daniel P. Berrangé]

On Tue, May 06, 2025 at 07:41:32PM +0300, Elisha Hollander wrote:
> Gave an example for a case where QEMU would try to allocate 0 bytes thus
> fail here in the original version of the patch.
> 
> > As I mentioned earlier, let's say you don't initialize the vertical
> display end registers, and set the minimum scanline register, the emulation
> will then have to allocate some display buffer, but because the vertical
> display end is initilized as 0 the buffer will be empty and the program
> break.

Isn't this an invalid hardware configuration that should be detected
in the emulation code, and either force the display end to a minimum
value, or trigger an assert ?

Patching a bug in a specific HW impl, by changing the qemu_memfd_alloc
code feels like it is probably the wrong place to address this.

> 
> I have no idea as for why my emails are getting messed up... :/
> 
> Have to go now, will try and send it again tomorrow probably...
> 
> On Tue, May 6, 2025, 19:37 Daniel P. Berrangé <berrange@redhat.com> wrote:
> 
> > On Tue, May 06, 2025 at 07:17:25PM +0300, Elisha Hollander wrote:
> > > Sorry for former patch something is messed up with my email.
> >
> > The commit message needs to explain what problem is being solved by
> > making this change as allowing 0 bytes looks dubious on the surface.
> >
> > >
> > > Signed-off-by: donno2048 <just4now666666@gmail.com>
> > > ---
> > >  util/memfd.c | 10 ++++++----
> > >  1 file changed, 6 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/util/memfd.c b/util/memfd.c
> > > index 8a2e906..e96e5af 100644
> > > --- a/util/memfd.c
> > > +++ b/util/memfd.c
> > > @@ -108,7 +108,7 @@ err:
> > >  void *qemu_memfd_alloc(const char *name, size_t size, unsigned int
> > seals,
> > >                         int *fd, Error **errp)
> > >  {
> > > - void *ptr;
> > > + void *ptr = NULL;
> > >      int mfd = qemu_memfd_create(name, size, false, 0, seals, NULL);
> > >
> > >      /* some systems have memfd without sealing */
> > > @@ -131,9 +131,11 @@ void *qemu_memfd_alloc(const char *name, size_t
> > size,
> > > unsigned int seals,
> > >          }
> > >      }
> > >
> > > - ptr = mmap(0, size, PROT_READ | PROT_WRITE, MAP_SHARED, mfd, 0);
> > > - if (ptr == MAP_FAILED) {
> > > - goto err;
> > > + if (size != 0) {
> > > + ptr = mmap(0, size, PROT_READ | PROT_WRITE, MAP_SHARED, mfd, 0);
> > > + if (ptr == MAP_FAILED) {
> > > + goto err;
> > > + }
> > >      }
> >
> > This patch is mangled.
> >
> >
> > With regards,
> > Daniel
> > --
> > |: https://berrange.com      -o-
> > https://www.flickr.com/photos/dberrange :|
> > |: https://libvirt.org         -o-
> > https://fstop138.berrange.com :|
> > |: https://entangle-photo.org    -o-
> > https://www.instagram.com/dberrange :|
> >
> >

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2] util/memfd: allow allocating 0 bytes

[Elisha Hollander]

[-- Attachment #1: Type: text/plain, Size: 3528 bytes --]

Maybe an assert is really more appropriate, but technically doing so on
actual hardware should run flawlessly so I think the emu should work too...
Maybe I'm wrong though

On Tue, May 6, 2025, 19:48 Daniel P. Berrangé <berrange@redhat.com> wrote:

> On Tue, May 06, 2025 at 07:41:32PM +0300, Elisha Hollander wrote:
> > Gave an example for a case where QEMU would try to allocate 0 bytes thus
> > fail here in the original version of the patch.
> >
> > > As I mentioned earlier, let's say you don't initialize the vertical
> > display end registers, and set the minimum scanline register, the
> emulation
> > will then have to allocate some display buffer, but because the vertical
> > display end is initilized as 0 the buffer will be empty and the program
> > break.
>
> Isn't this an invalid hardware configuration that should be detected
> in the emulation code, and either force the display end to a minimum
> value, or trigger an assert ?
>
> Patching a bug in a specific HW impl, by changing the qemu_memfd_alloc
> code feels like it is probably the wrong place to address this.
>
> >
> > I have no idea as for why my emails are getting messed up... :/
> >
> > Have to go now, will try and send it again tomorrow probably...
> >
> > On Tue, May 6, 2025, 19:37 Daniel P. Berrangé <berrange@redhat.com>
> wrote:
> >
> > > On Tue, May 06, 2025 at 07:17:25PM +0300, Elisha Hollander wrote:
> > > > Sorry for former patch something is messed up with my email.
> > >
> > > The commit message needs to explain what problem is being solved by
> > > making this change as allowing 0 bytes looks dubious on the surface.
> > >
> > > >
> > > > Signed-off-by: donno2048 <just4now666666@gmail.com>
> > > > ---
> > > >  util/memfd.c | 10 ++++++----
> > > >  1 file changed, 6 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/util/memfd.c b/util/memfd.c
> > > > index 8a2e906..e96e5af 100644
> > > > --- a/util/memfd.c
> > > > +++ b/util/memfd.c
> > > > @@ -108,7 +108,7 @@ err:
> > > >  void *qemu_memfd_alloc(const char *name, size_t size, unsigned int
> > > seals,
> > > >                         int *fd, Error **errp)
> > > >  {
> > > > - void *ptr;
> > > > + void *ptr = NULL;
> > > >      int mfd = qemu_memfd_create(name, size, false, 0, seals, NULL);
> > > >
> > > >      /* some systems have memfd without sealing */
> > > > @@ -131,9 +131,11 @@ void *qemu_memfd_alloc(const char *name, size_t
> > > size,
> > > > unsigned int seals,
> > > >          }
> > > >      }
> > > >
> > > > - ptr = mmap(0, size, PROT_READ | PROT_WRITE, MAP_SHARED, mfd, 0);
> > > > - if (ptr == MAP_FAILED) {
> > > > - goto err;
> > > > + if (size != 0) {
> > > > + ptr = mmap(0, size, PROT_READ | PROT_WRITE, MAP_SHARED, mfd, 0);
> > > > + if (ptr == MAP_FAILED) {
> > > > + goto err;
> > > > + }
> > > >      }
> > >
> > > This patch is mangled.
> > >
> > >
> > > With regards,
> > > Daniel
> > > --
> > > |: https://berrange.com      -o-
> > > https://www.flickr.com/photos/dberrange :|
> > > |: https://libvirt.org         -o-
> > > https://fstop138.berrange.com :|
> > > |: https://entangle-photo.org    -o-
> > > https://www.instagram.com/dberrange :|
> > >
> > >
>
> With regards,
> Daniel
> --
> |: https://berrange.com      -o-
> https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-
> https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-
> https://www.instagram.com/dberrange :|
>
>

[-- Attachment #2: Type: text/html, Size: 5763 bytes --]

Re: [PATCH v2] util/memfd: allow allocating 0 bytes

[Daniel P. Berrangé]

On Wed, May 07, 2025 at 01:25:34AM +0300, Elisha Hollander wrote:
> Maybe an assert is really more appropriate, but technically doing so on
> actual hardware should run flawlessly so I think the emu should work too...
> Maybe I'm wrong though

I'm still not clear which specific hardware device you're talking
about, but even if we don't want to assert, there's likely scope
for addressing the problem in that specific device rather than
changing the memfd code which has a semantic impact across all
users in qemu.

> 
> On Tue, May 6, 2025, 19:48 Daniel P. Berrangé <berrange@redhat.com> wrote:
> 
> > On Tue, May 06, 2025 at 07:41:32PM +0300, Elisha Hollander wrote:
> > > Gave an example for a case where QEMU would try to allocate 0 bytes thus
> > > fail here in the original version of the patch.
> > >
> > > > As I mentioned earlier, let's say you don't initialize the vertical
> > > display end registers, and set the minimum scanline register, the
> > emulation
> > > will then have to allocate some display buffer, but because the vertical
> > > display end is initilized as 0 the buffer will be empty and the program
> > > break.
> >
> > Isn't this an invalid hardware configuration that should be detected
> > in the emulation code, and either force the display end to a minimum
> > value, or trigger an assert ?
> >
> > Patching a bug in a specific HW impl, by changing the qemu_memfd_alloc
> > code feels like it is probably the wrong place to address this.
> >
> > >
> > > I have no idea as for why my emails are getting messed up... :/
> > >
> > > Have to go now, will try and send it again tomorrow probably...
> > >
> > > On Tue, May 6, 2025, 19:37 Daniel P. Berrangé <berrange@redhat.com>
> > wrote:
> > >
> > > > On Tue, May 06, 2025 at 07:17:25PM +0300, Elisha Hollander wrote:
> > > > > Sorry for former patch something is messed up with my email.
> > > >
> > > > The commit message needs to explain what problem is being solved by
> > > > making this change as allowing 0 bytes looks dubious on the surface.
> > > >
> > > > >
> > > > > Signed-off-by: donno2048 <just4now666666@gmail.com>
> > > > > ---
> > > > >  util/memfd.c | 10 ++++++----
> > > > >  1 file changed, 6 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/util/memfd.c b/util/memfd.c
> > > > > index 8a2e906..e96e5af 100644
> > > > > --- a/util/memfd.c
> > > > > +++ b/util/memfd.c
> > > > > @@ -108,7 +108,7 @@ err:
> > > > >  void *qemu_memfd_alloc(const char *name, size_t size, unsigned int
> > > > seals,
> > > > >                         int *fd, Error **errp)
> > > > >  {
> > > > > - void *ptr;
> > > > > + void *ptr = NULL;
> > > > >      int mfd = qemu_memfd_create(name, size, false, 0, seals, NULL);
> > > > >
> > > > >      /* some systems have memfd without sealing */
> > > > > @@ -131,9 +131,11 @@ void *qemu_memfd_alloc(const char *name, size_t
> > > > size,
> > > > > unsigned int seals,
> > > > >          }
> > > > >      }
> > > > >
> > > > > - ptr = mmap(0, size, PROT_READ | PROT_WRITE, MAP_SHARED, mfd, 0);
> > > > > - if (ptr == MAP_FAILED) {
> > > > > - goto err;
> > > > > + if (size != 0) {
> > > > > + ptr = mmap(0, size, PROT_READ | PROT_WRITE, MAP_SHARED, mfd, 0);
> > > > > + if (ptr == MAP_FAILED) {
> > > > > + goto err;
> > > > > + }
> > > > >      }
> > > >
> > > > This patch is mangled.
> > > >
> > > >
> > > > With regards,
> > > > Daniel
> > > > --
> > > > |: https://berrange.com      -o-
> > > > https://www.flickr.com/photos/dberrange :|
> > > > |: https://libvirt.org         -o-
> > > > https://fstop138.berrange.com :|
> > > > |: https://entangle-photo.org    -o-
> > > > https://www.instagram.com/dberrange :|
> > > >
> > > >
> >
> > With regards,
> > Daniel
> > --
> > |: https://berrange.com      -o-
> > https://www.flickr.com/photos/dberrange :|
> > |: https://libvirt.org         -o-
> > https://fstop138.berrange.com :|
> > |: https://entangle-photo.org    -o-
> > https://www.instagram.com/dberrange :|
> >
> >

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

[PATCH v2] util/memfd: allow allocating 0 bytes

[Elisha Hollander]

[-- Attachment #1: Type: text/plain, Size: 1286 bytes --]

> As I mentioned earlier, let's say you don't initialize the vertical
display end registers, and set the minimum scanline register, the emulation
will then have to allocate some display buffer, but because the vertical
display end is initilized as 0 the buffer will be empty and the program
break.

Signed-off-by: donno2048 <just4now666666@gmail.com>

---
 util/memfd.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/util/memfd.c b/util/memfd.c
index 8a2e906..e96e5af 100644
--- a/util/memfd.c
+++ b/util/memfd.c
@@ -108,7 +108,7 @@ err:
 void *qemu_memfd_alloc(const char *name, size_t size, unsigned int seals,
                        int *fd, Error **errp)
 {
-    void *ptr;
+    void *ptr = NULL;
     int mfd = qemu_memfd_create(name, size, false, 0, seals, NULL);

     /* some systems have memfd without sealing */
@@ -131,9 +131,11 @@ void *qemu_memfd_alloc(const char *name, size_t size,
unsigned int seals,
         }
     }

-    ptr = mmap(0, size, PROT_READ | PROT_WRITE, MAP_SHARED, mfd, 0);
-    if (ptr == MAP_FAILED) {
-        goto err;
+    if (size != 0) {
+        ptr = mmap(0, size, PROT_READ | PROT_WRITE, MAP_SHARED, mfd, 0);
+        if (ptr == MAP_FAILED) {
+            goto err;
+        }
     }

     *fd = mfd;
--
2.30.2

[-- Attachment #2: Type: text/html, Size: 2318 bytes --]

Re: [PATCH v2] util/memfd: allow allocating 0 bytes

[Philippe Mathieu-Daudé]

Hi Elisha,

On 6/5/25 18:44, Elisha Hollander wrote:
>  > As I mentioned earlier, let's say you don't initialize the vertical 

"As I mentioned earlier": where? Otherwise this description will be of
little relevance in 5 years from now in our history.

> display end registers, and set the minimum scanline register, the 
> emulation will then have to allocate some display buffer, but because 
> the vertical display end is initilized as 0 the buffer will be empty and 

Typo "initialized".

> the program break.
> 
> Signed-off-by: donno2048 <just4now666666@gmail.com 
> <mailto:just4now666666@gmail.com>>
> 
> ---
>   util/memfd.c | 10 ++++++----
>   1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/util/memfd.c b/util/memfd.c
> index 8a2e906..e96e5af 100644
> --- a/util/memfd.c
> +++ b/util/memfd.c
> @@ -108,7 +108,7 @@ err:
>   void *qemu_memfd_alloc(const char *name, size_t size, unsigned int seals,
>                          int *fd, Error **errp)
>   {
> -    void *ptr;
> +    void *ptr = NULL;
>       int mfd = qemu_memfd_create(name, size, false, 0, seals, NULL);
> 
>       /* some systems have memfd without sealing */
> @@ -131,9 +131,11 @@ void *qemu_memfd_alloc(const char *name, size_t 
> size, unsigned int seals,
>           }
>       }
> 
> -    ptr = mmap(0, size, PROT_READ | PROT_WRITE, MAP_SHARED, mfd, 0);
> -    if (ptr == MAP_FAILED) {
> -        goto err;
> +    if (size != 0) {
> +        ptr = mmap(0, size, PROT_READ | PROT_WRITE, MAP_SHARED, mfd, 0);
> +        if (ptr == MAP_FAILED) {
> +            goto err;
> +        }
>       }
> 
>       *fd = mfd;
> --
> 2.30.2

Alternatively always set *fd, removing the @err label:

-- >8 --
@@ -132,0 +133 @@ void *qemu_memfd_alloc(const char *name, size_t size, 
unsigned int seals,
+    *fd = mfd;
@@ -134,3 +135,2 @@ void *qemu_memfd_alloc(const char *name, size_t 
size, unsigned int seals,
-    ptr = mmap(0, size, PROT_READ | PROT_WRITE, MAP_SHARED, mfd, 0);
-    if (ptr == MAP_FAILED) {
-        goto err;
+    if (!size) {
+        return NULL;
@@ -139,2 +139,4 @@ void *qemu_memfd_alloc(const char *name, size_t 
size, unsigned int seals,
-    *fd = mfd;
-    return ptr;
+    ptr = mmap(0, size, PROT_READ | PROT_WRITE, MAP_SHARED, mfd, 0);
+    if (ptr != MAP_FAILED) {
+        return ptr;
+    }
@@ -142 +143,0 @@ void *qemu_memfd_alloc(const char *name, size_t size, 
unsigned int seals,
-err:
---

Or more similar to your approach:

-- >8 --
@@ -111 +111 @@ void *qemu_memfd_alloc(const char *name, size_t size, 
unsigned int seals,
-    void *ptr;
+    void *ptr = NULL;
@@ -134,5 +133,0 @@ void *qemu_memfd_alloc(const char *name, size_t 
size, unsigned int seals,
-    ptr = mmap(0, size, PROT_READ | PROT_WRITE, MAP_SHARED, mfd, 0);
-    if (ptr == MAP_FAILED) {
-        goto err;
-    }
-
@@ -140 +134,0 @@ void *qemu_memfd_alloc(const char *name, size_t size, 
unsigned int seals,
-    return ptr;
@@ -142,4 +136,2 @@ void *qemu_memfd_alloc(const char *name, size_t 
size, unsigned int seals,
-err:
-    error_setg_errno(errp, errno, "failed to allocate shared memory");
-    if (mfd >= 0) {
-        close(mfd);
+    if (!size) {
+        return NULL;
@@ -147 +139,11 @@ err:
-    return NULL;
+
+    if (size) {
+        ptr = mmap(0, size, PROT_READ | PROT_WRITE, MAP_SHARED, mfd, 0);
+        if (ptr == MAP_FAILED) {
+            error_setg_errno(errp, errno, "failed to allocate shared 
memory");
+            if (mfd >= 0) {
+                close(mfd);
+            }
+        }
+    }
+    return ptr;
---

Re: [PATCH v2] util/memfd: allow allocating 0 bytes

[Elisha Hollander]

[-- Attachment #1: Type: text/plain, Size: 4160 bytes --]

> "As I mentioned earlier": where?
> Otherwise this description will be of
> little relevance in 5 years from now in our history.

I explained the motivation on the first revision of the patch

On Wed, May 7, 2025, 14:58 Philippe Mathieu-Daudé <philmd@linaro.org> wrote:

> Hi Elisha,
>
> On 6/5/25 18:44, Elisha Hollander wrote:
> >  > As I mentioned earlier, let's say you don't initialize the vertical
>
> "As I mentioned earlier": where? Otherwise this description will be of
> little relevance in 5 years from now in our history.
>
> > display end registers, and set the minimum scanline register, the
> > emulation will then have to allocate some display buffer, but because
> > the vertical display end is initilized as 0 the buffer will be empty and
>
> Typo "initialized".
>
> > the program break.
> >
> > Signed-off-by: donno2048 <just4now666666@gmail.com
> > <mailto:just4now666666@gmail.com>>
> >
> > ---
> >   util/memfd.c | 10 ++++++----
> >   1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/util/memfd.c b/util/memfd.c
> > index 8a2e906..e96e5af 100644
> > --- a/util/memfd.c
> > +++ b/util/memfd.c
> > @@ -108,7 +108,7 @@ err:
> >   void *qemu_memfd_alloc(const char *name, size_t size, unsigned int
> seals,
> >                          int *fd, Error **errp)
> >   {
> > -    void *ptr;
> > +    void *ptr = NULL;
> >       int mfd = qemu_memfd_create(name, size, false, 0, seals, NULL);
> >
> >       /* some systems have memfd without sealing */
> > @@ -131,9 +131,11 @@ void *qemu_memfd_alloc(const char *name, size_t
> > size, unsigned int seals,
> >           }
> >       }
> >
> > -    ptr = mmap(0, size, PROT_READ | PROT_WRITE, MAP_SHARED, mfd, 0);
> > -    if (ptr == MAP_FAILED) {
> > -        goto err;
> > +    if (size != 0) {
> > +        ptr = mmap(0, size, PROT_READ | PROT_WRITE, MAP_SHARED, mfd, 0);
> > +        if (ptr == MAP_FAILED) {
> > +            goto err;
> > +        }
> >       }
> >
> >       *fd = mfd;
> > --
> > 2.30.2
>
> Alternatively always set *fd, removing the @err label:
>
> -- >8 --
> @@ -132,0 +133 @@ void *qemu_memfd_alloc(const char *name, size_t size,
> unsigned int seals,
> +    *fd = mfd;
> @@ -134,3 +135,2 @@ void *qemu_memfd_alloc(const char *name, size_t
> size, unsigned int seals,
> -    ptr = mmap(0, size, PROT_READ | PROT_WRITE, MAP_SHARED, mfd, 0);
> -    if (ptr == MAP_FAILED) {
> -        goto err;
> +    if (!size) {
> +        return NULL;
> @@ -139,2 +139,4 @@ void *qemu_memfd_alloc(const char *name, size_t
> size, unsigned int seals,
> -    *fd = mfd;
> -    return ptr;
> +    ptr = mmap(0, size, PROT_READ | PROT_WRITE, MAP_SHARED, mfd, 0);
> +    if (ptr != MAP_FAILED) {
> +        return ptr;
> +    }
> @@ -142 +143,0 @@ void *qemu_memfd_alloc(const char *name, size_t size,
> unsigned int seals,
> -err:
> ---
>
> Or more similar to your approach:
>
> -- >8 --
> @@ -111 +111 @@ void *qemu_memfd_alloc(const char *name, size_t size,
> unsigned int seals,
> -    void *ptr;
> +    void *ptr = NULL;
> @@ -134,5 +133,0 @@ void *qemu_memfd_alloc(const char *name, size_t
> size, unsigned int seals,
> -    ptr = mmap(0, size, PROT_READ | PROT_WRITE, MAP_SHARED, mfd, 0);
> -    if (ptr == MAP_FAILED) {
> -        goto err;
> -    }
> -
> @@ -140 +134,0 @@ void *qemu_memfd_alloc(const char *name, size_t size,
> unsigned int seals,
> -    return ptr;
> @@ -142,4 +136,2 @@ void *qemu_memfd_alloc(const char *name, size_t
> size, unsigned int seals,
> -err:
> -    error_setg_errno(errp, errno, "failed to allocate shared memory");
> -    if (mfd >= 0) {
> -        close(mfd);
> +    if (!size) {
> +        return NULL;
> @@ -147 +139,11 @@ err:
> -    return NULL;
> +
> +    if (size) {
> +        ptr = mmap(0, size, PROT_READ | PROT_WRITE, MAP_SHARED, mfd, 0);
> +        if (ptr == MAP_FAILED) {
> +            error_setg_errno(errp, errno, "failed to allocate shared
> memory");
> +            if (mfd >= 0) {
> +                close(mfd);
> +            }
> +        }
> +    }
> +    return ptr;
> ---
>

[-- Attachment #2: Type: text/html, Size: 5293 bytes --]

[PATCH v2] util/memfd: allow allocating 0 bytes

[Elisha Hollander]

[-- Attachment #1: Type: text/plain, Size: 932 bytes --]

Signed-off-by: donno2048 <just4now666666@gmail.com>
---
 util/memfd.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/util/memfd.c b/util/memfd.c
index 8a2e906..e96e5af 100644
--- a/util/memfd.c
+++ b/util/memfd.c @@ -108,7 +108,7 @@ err:
 void *qemu_memfd_alloc(const char *name, size_t size, unsigned int seals,
                        int *fd, Error **errp)
 { - void *ptr;
+ void *ptr = NULL;
     int mfd = qemu_memfd_create(name, size, false, 0, seals, NULL);

     /* some systems have memfd without sealing */
@@ -131,9 +131,11 @@ void *qemu_memfd_alloc(const char *name, size_t size,
unsigned int seals,
         }
     }

- ptr = mmap(0, size, PROT_READ | PROT_WRITE, MAP_SHARED, mfd, 0);
- if (ptr == MAP_FAILED) { - goto err;
+ if (size != 0) {
+ ptr = mmap(0, size, PROT_READ | PROT_WRITE, MAP_SHARED, mfd, 0); + if
(ptr == MAP_FAILED) {
+ goto err;
+ }
     }

     *fd = mfd;
--
2.30.2

[-- Attachment #2: Type: text/html, Size: 2065 bytes --]