From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id B148CC433EF for ; Mon, 23 May 2022 18:24:47 +0000 (UTC) Received: from localhost ([::1]:57174 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1ntCjW-0003zE-QL for qemu-devel@archiver.kernel.org; Mon, 23 May 2022 14:24:46 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:39444) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1ntCOU-000225-BL for qemu-devel@nongnu.org; Mon, 23 May 2022 14:03:02 -0400 Received: from mail-lj1-x22f.google.com ([2a00:1450:4864:20::22f]:33395) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1ntCOR-0004TV-66 for qemu-devel@nongnu.org; Mon, 23 May 2022 14:03:01 -0400 Received: by mail-lj1-x22f.google.com with SMTP id 27so13205053ljw.0 for ; Mon, 23 May 2022 11:02:58 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=MH+M8Bh0CbR9eUgXzdNFfMgvhBhrHXgI9Ctwz+nFHl4=; b=oZMsBRj/kE1aQ0oA9aSuahJGK5rl8h/QEWGwn0BXiReD92Eyldb565s44GqBEgAO3A Yq+HcVWK7dI7cojg2O3V2/m3Ibw7Fd5/3kGatEWkNvpfXJYRhy+J4WPSLIWhwbx9RPtK 2a2H1pjm6IgyDdFdGNSD0vTCpUELwwpOhEtAlNSXQUaC3VGSoE9+BbixItux/WQ3LkuG BS1jkPeBsm7hqHDkRWDLahIzDn6AZBuuv7xy3eSkEZHroQy2M64966fKHlnGks6XbgXX 7Xl5Fvgpy0/KxTGDpT82BgVrsBcUNYULPgQ60v/v0ODaaXDj7+0tj1uTtNY28fCZ8fpX 36tQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=MH+M8Bh0CbR9eUgXzdNFfMgvhBhrHXgI9Ctwz+nFHl4=; b=2UcYlgKK7eGHSUnBYTruB+lNZYnLQEaU95HiwrOlO+jB4lLy25yhWMjvAY9EeEePs8 5qMTXVfewoxLbquxMC7GU9abG1abf5C0abm7AQvunUhUrlT8xy8IzjHkjWbD5sxHLwsI +rcncStNHo77rtSbdDlL+2WjaMXqLvefO0iHk7kfxHGg0p4pKNnY9ZZYTwEA9H30M033 PapS37N9FFLxYf3eK4aFBLaOteKAQdBtg4ZXPkp4ZBvzj482KCARJnwUoxEhRnZtsKLE lItW93cDA2tk4y+o+9h72C3BC7A/0EdLFdHqQmOWIew3fF1ArKK4+8gJf4xbLXoZUchs JHiA== X-Gm-Message-State: AOAM532V6FUv4MnAJjDjTNoXrr+mGi7nTcAkIKzpZEGjZd1J2ERM8yuN IckAz636JG4EaVCd8Ok6AnFOVh/BqvSNk4lrHvk= X-Google-Smtp-Source: ABdhPJzcQrb4IcuCsuqylhivPFgD1Rnu5eGyEPRCF8lHTe6zgvYpwdxnk2O9+sTEsE4H7LMSg43WcEgPdBFtP6S08Fk= X-Received: by 2002:a2e:9ec9:0:b0:253:e132:6e3d with SMTP id h9-20020a2e9ec9000000b00253e1326e3dmr8027191ljk.345.1653328977307; Mon, 23 May 2022 11:02:57 -0700 (PDT) MIME-Version: 1.0 References: <20220513180821.905149-1-marcandre.lureau@redhat.com> <20220513180821.905149-6-marcandre.lureau@redhat.com> In-Reply-To: From: =?UTF-8?B?TWFyYy1BbmRyw6kgTHVyZWF1?= Date: Mon, 23 May 2022 20:02:45 +0200 Message-ID: Subject: Re: [PATCH v3 05/15] osdep: export qemu_open_cloexec() To: =?UTF-8?Q?Daniel_P=2E_Berrang=C3=A9?= Cc: QEMU , Konstantin Kostiuk , Michael Roth , Markus Armbruster Content-Type: multipart/alternative; boundary="00000000000063af1105dfb1a618" Received-SPF: pass client-ip=2a00:1450:4864:20::22f; envelope-from=marcandre.lureau@gmail.com; helo=mail-lj1-x22f.google.com X-Spam_score_int: -20 X-Spam_score: -2.1 X-Spam_bar: -- X-Spam_report: (-2.1 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, FREEMAIL_FROM=0.001, HTML_MESSAGE=0.001, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" --00000000000063af1105dfb1a618 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi On Mon, May 23, 2022 at 7:56 PM Daniel P. Berrang=C3=A9 wrote: > On Mon, May 23, 2022 at 07:30:42PM +0200, Marc-Andr=C3=A9 Lureau wrote: > > Hi > > > > On Mon, May 23, 2022 at 2:43 PM Daniel P. Berrang=C3=A9 > > wrote: > > > > > On Fri, May 13, 2022 at 08:08:11PM +0200, marcandre.lureau@redhat.com > > > wrote: > > > > From: Marc-Andr=C3=A9 Lureau > > > > > > > > Used in the next patch, to simplify qga code. > > > > > > > > Signed-off-by: Marc-Andr=C3=A9 Lureau > > > > --- > > > > 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 t= he > > > '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 ? eg put 'bool allow_fdset =3D true" in softmmu/vl.c, and > 'bool allow_fdset =3D false' in stubs/open.c, and then make > qemu_open_internal conditionalize itself on this global > variable, so only the system emulators get fdset support > activated. > > 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 :| > > --=20 Marc-Andr=C3=A9 Lureau --00000000000063af1105dfb1a618 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi

On Mon, May 23, 2022 at 7:56 PM Dan= iel P. Berrang=C3=A9 <berrange@re= dhat.com> wrote:
On Mon, May 23, 2022 at 07:30:42PM +0200, Marc-Andr=C3=A9 Lureau wr= ote:
> Hi
>
> On Mon, May 23, 2022 at 2:43 PM Daniel P. Berrang=C3=A9 <berrange@redhat.com><= br> > wrote:
>
> > On Fri, May 13, 2022 at 08:08:11PM +0200, marcandre.lureau@redhat.com > > wrote:
> > > From: Marc-Andr=C3=A9 Lureau <marcandre.lureau@redhat.com> > > >
> > > Used in the next patch, to simplify qga code.
> > >
> > > Signed-off-by: Marc-Andr=C3=A9 Lureau <marcandre.lureau@redhat.com<= /a>>
> > > ---
> > >=C2=A0 include/qemu/osdep.h |=C2=A0 1 +
> > >=C2=A0 util/osdep.c=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0| 10 +++= +++++--
> > >=C2=A0 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,
> > >=C2=A0 =C2=A0*/
> > >=C2=A0 int qemu_open_old(const char *name, int flags, ...); > > >=C2=A0 int qemu_open(const char *name, int flags, Error **err= p);
> > > +int qemu_open_cloexec(const char *name, int flags, mode_t m= ode, Error
> > **errp);
> >
> > I don't think we should be exporting this - it is just a vari= ant of the
> > 'qemu_open_old' method that we wanted callers to stop usi= ng in favour
> > of explicitly deciding between 'qemu_open' and 'qemu_= create'.
> >
>
>
> qemu_open() has "/dev/fdset" handling, which qemu-ga and oth= er tools don't
> need.

Right, but exporting this as 'qemu_open_cloexec' is going to mislea= d
people into thinking it is a better version of 'qemu_open'. This wi= ll
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.



eg put=C2=A0 'bool allow_fdset =3D true"=C2=A0 =C2=A0in softmmu/vl= .c, and
'bool allow_fdset =3D false' in stubs/open.c, and then make
qemu_open_internal conditionalize itself on this global
variable, so only the system emulators get fdset support
activated.

With regards,
Daniel
--
|: ht= tps://berrange.com=C2=A0 =C2=A0 =C2=A0 -o-=C2=A0 =C2=A0 h= ttps://www.flickr.com/photos/dberrange :|
|: htt= ps://libvirt.org=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0-o-=C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 https://fstop138.berrange.com :|
|: https://entangle-photo.org=C2=A0 =C2=A0 -o-=C2=A0 =C2=A0 = https://www.instagram.com/dberrange :|



--
Marc-Andr=C3=A9 Lureau
--00000000000063af1105dfb1a618--