Re: [Qemu-devel] [PATCH v2 2/2] hostmem-file: add 'sync' option

[Haozhong Zhang]

On 01/11/18 20:06 +0000, Dr. David Alan Gilbert wrote:
> * Haozhong Zhang (address@hidden) wrote:
> > This option controls whether QEMU mmap(2) the memory backend file with
> > MAP_SYNC flag, which can fully guarantee the guest write persistence
> > to the backend, if MAP_SYNC flag is supported by the host kernel
> > (Linux kernel 4.15 and later) and the backend is a file supporting
> > DAX (e.g., file on ext4/xfs file system mounted with '-o dax').
> > 
> > It can take one of following values:
> >  - on:  try to pass MAP_SYNC to mmap(2); if MAP_SYNC is not supported or
> >         'share=off', QEMU will abort
> >  - off: never pass MAP_SYNC to mmap(2)
> >  - auto (default): if MAP_SYNC is supported and 'share=on', work as if
> >         'sync=on'; otherwise, work as if 'sync=off'
> 
> I don't know anything about MAP_SYNC but see two minor comments below:
> 
> > Signed-off-by: Haozhong Zhang <address@hidden>
> > Sugguested-by: Eduardo Habkost <address@hidden>
>       ^ typo!
>
[..]
> > +static void file_memory_backend_set_sync(
> > +    Object *obj, Visitor *v, const char *name, void *opaque, Error **errp)
> > +{
> > +    HostMemoryBackend *backend = MEMORY_BACKEND(obj);
> > +    HostMemoryBackendFile *fb = MEMORY_BACKEND_FILE(obj);
> > +    Error *local_err = NULL;
> > +    OnOffAuto value;
> > +
> > +    if (host_memory_backend_mr_inited(backend)) {
> > +        error_setg(&local_err, "cannot change property value");
> 
> Please make this error clearer; if I was helping someone and saw
> this error message in a log, I wouldn't know what type of property it
> was on what device.   Please at least include something saying it's
> memory backend related and sync related; even just including a %s and
> __func__ would help;  even better if you include the name of the object
> so if you had multiple backend files and one failed you could see which
> one.

I'll fix the typo, and include the backend id and the property name in
the error messages in the next version.

Thanks,
Haozhong