Hi

On Mon, May 23, 2022 at 8:11 PM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Mon, May 23, 2022 at 08:02:45PM +0200, Marc-André Lureau wrote:
> > Hi
> >
> > On Mon, May 23, 2022 at 7:56 PM Daniel P. Berrangé <berrange@redhat.com>
> > wrote:
> >
> > > On Mon, May 23, 2022 at 07:30:42PM +0200, Marc-André Lureau wrote:
> > > > Hi
> > > >
> > > > On Mon, May 23, 2022 at 2:43 PM Daniel P. Berrangé <
> berrange@redhat.com>
> > > > wrote:
> > > >
> > > > > On Fri, May 13, 2022 at 08:08:11PM +0200,
> marcandre.lureau@redhat.com
> > > > > wrote:
> > > > > > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > > >
> > > > > > Used in the next patch, to simplify qga code.
> > > > > >
> > > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > > > ---
> > > > > >  include/qemu/osdep.h |  1 +
> > > > > >  util/osdep.c         | 10 ++++++++--
> > > > > >  2 files changed, 9 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> > > > > > index 67cc465416..64f51cfb7a 100644
> > > > > > --- a/include/qemu/osdep.h
> > > > > > +++ b/include/qemu/osdep.h
> > > > > > @@ -489,6 +489,7 @@ void sigaction_invoke(struct sigaction
> *action,
> > > > > >   */
> > > > > >  int qemu_open_old(const char *name, int flags, ...);
> > > > > >  int qemu_open(const char *name, int flags, Error **errp);
> > > > > > +int qemu_open_cloexec(const char *name, int flags, mode_t mode,
> > > Error
> > > > > **errp);
> > > > >
> > > > > I don't think we should be exporting this - it is just a variant
> of the
> > > > > 'qemu_open_old' method that we wanted callers to stop using in
> favour
> > > > > of explicitly deciding between 'qemu_open' and 'qemu_create'.
> > > > >
> > > >
> > > >
> > > > qemu_open() has "/dev/fdset" handling, which qemu-ga and other tools
> > > don't
> > > > need.
> > >
> > > Right, but exporting this as 'qemu_open_cloexec' is going to mislead
> > > people into thinking it is a better version of 'qemu_open'. This will
> > > cause us to loose support for /dev/fdset in places where we actually
> > > need it.
> > >
> >
> > > It is pretty harmless to have /dev/fdset there, even if the tool does
> > > not need it - that's been the case with many QEMU tools for many years.
> > > If we think it is actually a real problem though, we should just have
> > > a way to toggle it on/off from the existing APIs.
> > >
> > >
> > It's a bit problematic to make qemu-ga standalone, and have a common
> shared
> > subproject/library.
> >
> > Maybe introduce a callback for QEMU/QMP "/dev/fdset" handling ? any
> better
> > idea ?
>
> If we want to make qemu-ga standalone, then IMHO we should be
> aggressively switching it to use as many GLib APIs as possible,
> eliminating its reliance on any of QEMU's home-grown portability
> functions. All the 'FILE *' / 'open' scenarios could be replaced
> with GIO's GFile/GInputStream/GOutputStream for example.
>

I am not too eager to do that kind of refactoring. Even rewriting in Rust
seems a bit pointless to me, even if I would have more motivation.

Also there are times you do open() for things that are not stream-related.
And glib sadly doesn't really offer a solution for open(CLOEXEC).

I guess I can simply add an open_cloexec() helper function in qemu-ga alone
for now.