Re: [PATCH 1/7] backends/hostmem: Drop redundant validation of property "size"

["Daniel P. Berrangé" <berrange@redhat.com>]

On Thu, Nov 12, 2026 at 08:44:28AM +0100, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Wed, Nov 11, 2026 at 01:23:36PM +0100, Markus Armbruster wrote:
> >> Property "size" is mandatory, and its setter
> >> host_memory_backend_set_size() rejects value zero.  Delete the
> >> redundant checks for zero in the .alloc() callbacks.
> >> 
> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >> ---
> >>  backends/hostmem-epc.c   | 5 -----
> >>  backends/hostmem-file.c  | 4 ----
> >>  backends/hostmem-memfd.c | 5 -----
> >>  backends/hostmem-ram.c   | 5 -----
> >>  backends/hostmem-shm.c   | 5 -----
> >>  5 files changed, 24 deletions(-)
> >
> > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> >
> > ...it is a shame this was/is dead code, since the actual error message
> > from host_memory_backend_set_size() is (IMHO) worse than that from the
> > unreachable code you're deleting
> >
> >   $ ./build/qemu-system-x86_64 -object memory-backend-ram,size=0,id=foo
> >   qemu-system-x86_64: property 'size' of memory-backend-ram doesn't take value '0'
> 
> Fair point.
> 
> > as a separate patch I might suggest
> >
> >     diff --git a/backends/hostmem.c b/backends/hostmem.c
> >     index 173d042be35..f523dce6eb0 100644
> >     --- a/backends/hostmem.c
> >     +++ b/backends/hostmem.c
> >     @@ -77,8 +77,8 @@ host_memory_backend_set_size(Object *obj, Visitor *v, const char *name,
>            if (host_memory_backend_mr_inited(backend)) {
>                error_setg(errp, "cannot change property %s of %s ", name,
>                           object_get_typename(obj));
>                return;
>            }
> 
>            if (!visit_type_size(v, name, &value, errp)) {
>                return;
> >          }
> >          if (!value) {
> >              error_setg(errp,
> >     -                   "property '%s' of %s doesn't take value '%" PRIu64 "'",
> >     -                   name, object_get_typename(obj), value);
> >     +                   "cannot create '%s' with a size of 0",
> >     +                   object_get_typename(obj));
> >              return;
> >          }
> >          backend->size = value;
> >
> > so that we get the nicer message:
> >
> >     $ ./build/qemu-system-x86_64 -object memory-backend-ram,size=0,id=foo
> >     qemu-system-x86_64: cannot create 'memory-backend-ram' with a size of 0
> 
> Your message assumes that the property is only ever set on behalf of an
> object creation request.  Not true in general, qom-set exists.  I
> believe it's true here due to the host_memory_backend_mr_inited() guard.
> But that's not entirely obvious, isn't it?
> 
> Possible phrasings that don't assume this:
> 
>    property 'size' of 'memory-backend-ram' cannot be zero
>    cannot set property 'size' of 'memory-backend-ram' to zero
> 
> Is one of them good enough?  I doubt this error is common.

I'm fine with either of those, perhaps mildly leaning towards
the second.

> There are enough "doesn't take value" error messages elsewhere to
> justify a small series of its own.


With regards,
Daniel
-- 
|: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
|: https://libvirt.org          ~~          https://entangle-photo.org :|
|: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|