Re: [PATCH v2 13/21] hw/uefi: add var-service-json.c + qapi for NV vars.

[Gerd Hoffmann]

> > +    lseek(uv->jsonfd, 0, SEEK_SET);
> > +    rc = write(uv->jsonfd, gstr->str, gstr->len);
> > +    if (rc != gstr->len) {
> > +        warn_report("%s: write error", __func__);
> > +    }
> > +    rc = ftruncate(uv->jsonfd, gstr->len);
> > +    if (rc != 0) {
> > +        warn_report("%s: ftruncate error", __func__);
> > +    }
> > +    fsync(uv->jsonfd);
> 
> Although the fsync helps, re-writing the file in-place is a ad idea for data
> integrity on host OS crash. Especially if the new data is shorter, we would
> easily end up with a file containing old and new data making it unparsable
> (assuming the parser doesn't ignore trailing data).

I've tried to minimize the number of syscalls for the update, hoping to
also minimize the chance for corruption.

> I'd like to suggest the write to temp file + rename dance to get atomic
> replacement. The problem with that is that it hits the DAC/MAC security
> restrictions we put QEMU under :-(

Yep.  If we want allow libvirt passing file handles to qemu we can't do
the rename dance.

> My next best idea is to re-arrange things thus:
> 
>    lseek(fd, 0)
>    ftruncate(fd, 0)
>    fsync(fd)
>    write(fd, str)
>    fsync(fd)
> 
> so we should at least reduce the liklihood of getting a mix of
> old and new data - empty file is better than a mix of data.

On parse errors the complete file content is simply ignored, so it
should not make much of a difference whenever the file is empty or
corrupted.

Reorder the calls to first ftruncate then write looks reasonable.
Not sure the extra fsync makes sense.

take care,
  Gerd