On Thu, Dec 19, 2024 at 11:49:49AM -0600, Michael Roth wrote:
> On Thu, Dec 19, 2024 at 01:37:18PM +0000, Daniel P. Berrangé wrote:
> > IMHO we msut consider unlink() to be a valid thing, because the right
> > way for apps to perform crash safe atomic updates of existing files,
> > is to use rename() from a temporary file, and the rename() in has an
> > implicit unlink as part of its operation. ie apps would be doing:
> >
> > fd = open("foo.tmp")
> > write(fd, ...)
> > fsync(fd)
> > close(fd)
> > rename("foo.tmp", "foo")
>
> If we still want to allow for this rather than enforcing in-place
> update, one alternative would be to just allow a separate lock file
> to be specified rather than locking the certificate file itself. That
> would provide a bit more flexibility.
>
> I can update the QEMU implementation to take -certs-lock-file in
> addition to -certs-file so they can be specified separately. And if
> -certs-lock-file is not specified then QEMU will just assume
> management handles things different or has agreed to not do endorsement
> key updates while SNP guests are running.
>
> I think we'd considered something like that originally but the thinking
> was that locking the certs themselves was more organic in terms of an
> "obvious"/natural solution. But it does end up being a bit more
> inflexible WRT how libraries/etc. might manage file updates underneath
> the covers, so maybe a lock file is the better approach after all.
If we want locking, I think locking the certs file directly is a nicer
idea, as it avoids everyone having to agree on the location of the
lock file, relative to the certs file.
The current locking code just needs to go inside a while(1) loop and
have fstat + stat added to detect the recreation race. For example:
https://gitlab.com/libvirt/libvirt/-/blob/master/src/util/virpidfile.c#L376
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 :|
