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

[Daniel P . Berrangé]

On Tue, Jan 07, 2025 at 04:33:40PM +0100, Gerd Hoffmann wrote:
> Define qapi schema for the uefi variable store state.
> 
> Use it and the generated visitor helper functions to store persistent
> (EFI_VARIABLE_NON_VOLATILE) variables in JSON format on disk.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/uefi/var-service-json.c | 242 +++++++++++++++++++++++++++++++++++++
>  qapi/meson.build           |   1 +
>  qapi/qapi-schema.json      |   1 +
>  qapi/uefi.json             |  45 +++++++
>  4 files changed, 289 insertions(+)
>  create mode 100644 hw/uefi/var-service-json.c
>  create mode 100644 qapi/uefi.json
> 

> +void uefi_vars_json_init(uefi_vars_state *uv, Error **errp)
> +{
> +    if (uv->jsonfile) {
> +        uv->jsonfd = qemu_create(uv->jsonfile, O_RDWR, 0666, errp);
> +    }
> +}
> +
> +void uefi_vars_json_save(uefi_vars_state *uv)
> +{
> +    GString *gstr;
> +    int rc;
> +
> +    if (uv->jsonfd == -1) {
> +        return;
> +    }
> +
> +    gstr = uefi_vars_to_json(uv);
> +
> +    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'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 :-(

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.



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 :|