Hi
On Tue, Aug 28, 2018 at 5:53 PM Daniel P. Berrangé <berra...@redhat.com> wrote:
>
> On Fri, Jul 13, 2018 at 03:09:07PM +0200, Marc-André Lureau wrote:
> > There are variants of qemu_create_pidfile() in qemu-pr-helper and
> > qemu-ga. Let's have a common implementation in libqemuutil.
> >
> > The code is based from pr-helper write_pidfile(), but allows the
> > caller to deal with error reporting and behaviour.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com>
> > ---
> > include/qemu/osdep.h | 3 ++-
> > os-posix.c | 24 -------------------
> > os-win32.c | 25 --------------------
> > qga/main.c | 54 ++++++++-----------------------------------
> > scsi/qemu-pr-helper.c | 40 ++++----------------------------
> > util/oslib-posix.c | 33 ++++++++++++++++++++++++++
> > util/oslib-win32.c | 27 ++++++++++++++++++++++
> > vl.c | 4 ++--
> > 8 files changed, 79 insertions(+), 131 deletions(-)
>
>
>
> > diff --git a/util/oslib-posix.c b/util/oslib-posix.c
> > index 13b6f8d776..da1d4a3201 100644
> > --- a/util/oslib-posix.c
> > +++ b/util/oslib-posix.c
> > @@ -88,6 +88,39 @@ int qemu_daemon(int nochdir, int noclose)
> > return daemon(nochdir, noclose);
> > }
> >
> > +bool qemu_write_pidfile(const char *pidfile, Error **errp)
> > +{
> > + int pidfd;
> > + char pidstr[32];
> > +
> > + pidfd = qemu_open(pidfile, O_CREAT | O_WRONLY, S_IRUSR | S_IWUSR);
> > + if (pidfd == -1) {
> > + error_setg_errno(errp, errno, "Cannot open pid file");
> > + return false;
> > + }
> > +
> > + if (lockf(pidfd, F_TLOCK, 0)) {
> > + error_setg_errno(errp, errno, "Cannot lock pid file");
> > + goto fail;
> > + }
> > + if (ftruncate(pidfd, 0)) {
> > + error_setg_errno(errp, errno, "Failed to truncate pid file");
> > + goto fail;
> > + }
> > +
> > + snprintf(pidstr, sizeof(pidstr), "%d\n", getpid());
> > + if (write(pidfd, pidstr, strlen(pidstr)) != strlen(pidstr)) {
> > + error_setg(errp, "Failed to write pid file");
> > + goto fail;
> > + }
> > + return true;
> > +
> > +fail:
> > + unlink(pidfile);
>
> Danger, Will Robinson !
>
> We can get to this fail: label if we were unable to lockf() the
> pidfile. ie someone else owns the pidfile, and we've now unlinked
> the pidfile they own.
The code was based on qemu-pr-helper, so the problem exists there.
So we better follow ga_open_pidfile() version here, and close the fd,
return an error. ok
>
>
> > + close(pidfd);
> > + return false;
> > +}
> > +
> > void *qemu_oom_check(void *ptr)
> > {
> > if (ptr == NULL) {
>
>
> 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 :|
>
--
Marc-André Lureau
