DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 5F6C322B4B
Date: Thu, 22 Jun 2017 11:29:44 -0700 (PDT)
From: Stefano Stabellini <sstabellini@kernel.org>
To: =?UTF-8?Q?Roger_Pau_Monn=C3=A9?= <roger.pau@citrix.com>
cc: Stefano Stabellini <sstabellini@kernel.org>, xen-devel@lists.xen.org,
        jgross@suse.com, Stefano Stabellini <stefano@aporeto.com>,
        boris.ostrovsky@oracle.com, linux-kernel@vger.kernel.org
Subject: Re: [Xen-devel] [PATCH v4 07/18] xen/pvcalls: implement socket
 command
In-Reply-To: <20170622082632.atg6dslz5zs2gtco@dhcp-3-128.uk.xensource.com>
Message-ID: <alpine.DEB.2.10.1706221118110.12819@sstabellini-ThinkPad-X260>
References: <alpine.DEB.2.10.1706151156350.12156@sstabellini-ThinkPad-X260> <1497553787-3709-1-git-send-email-sstabellini@kernel.org> <1497553787-3709-7-git-send-email-sstabellini@kernel.org> <20170620161112.7x4qickboyt7qyvh@dhcp-3-128.uk.xensource.com>
 <alpine.DEB.2.10.1706211312450.12819@sstabellini-ThinkPad-X260> <20170622082632.atg6dslz5zs2gtco@dhcp-3-128.uk.xensource.com>
User-Agent: Alpine 2.10 (DEB 1266 2009-07-14)
MIME-Version: 1.0
Content-Type: MULTIPART/MIXED; BOUNDARY="8323329-1888458059-1498156185=:12819"
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4289
Lines: 94

  This message is in MIME format.  The first part should be readable text,
  while the remaining parts are likely unreadable without MIME-aware tools.

--8323329-1888458059-1498156185=:12819
Content-Type: TEXT/PLAIN; charset=UTF-8
Content-Transfer-Encoding: 8BIT

On Thu, 22 Jun 2017, Roger Pau Monné wrote:
> On Wed, Jun 21, 2017 at 01:16:56PM -0700, Stefano Stabellini wrote:
> > On Tue, 20 Jun 2017, Roger Pau Monné wrote:
> > > On Thu, Jun 15, 2017 at 12:09:36PM -0700, Stefano Stabellini wrote:
> > > > Just reply with success to the other end for now. Delay the allocation
> > > > of the actual socket to bind and/or connect.
> > > > 
> > > > Signed-off-by: Stefano Stabellini <stefano@aporeto.com>
> > > > CC: boris.ostrovsky@oracle.com
> > > > CC: jgross@suse.com
> > > > ---
> > > >  drivers/xen/pvcalls-back.c | 27 +++++++++++++++++++++++++++
> > > >  1 file changed, 27 insertions(+)
> > > > 
> > > > diff --git a/drivers/xen/pvcalls-back.c b/drivers/xen/pvcalls-back.c
> > > > index 437c2ad..953458b 100644
> > > > --- a/drivers/xen/pvcalls-back.c
> > > > +++ b/drivers/xen/pvcalls-back.c
> > > > @@ -12,12 +12,17 @@
> > > >   * GNU General Public License for more details.
> > > >   */
> > > >  
> > > > +#include <linux/inet.h>
> > > >  #include <linux/kthread.h>
> > > >  #include <linux/list.h>
> > > >  #include <linux/radix-tree.h>
> > > >  #include <linux/module.h>
> > > >  #include <linux/semaphore.h>
> > > >  #include <linux/wait.h>
> > > > +#include <net/sock.h>
> > > > +#include <net/inet_common.h>
> > > > +#include <net/inet_connection_sock.h>
> > > > +#include <net/request_sock.h>
> > > >  
> > > >  #include <xen/events.h>
> > > >  #include <xen/grant_table.h>
> > > > @@ -54,6 +59,28 @@ struct pvcalls_fedata {
> > > >  static int pvcalls_back_socket(struct xenbus_device *dev,
> > > >  		struct xen_pvcalls_request *req)
> > > >  {
> > > > +	struct pvcalls_fedata *fedata;
> > > > +	int ret;
> > > > +	struct xen_pvcalls_response *rsp;
> > > > +
> > > > +	fedata = dev_get_drvdata(&dev->dev);
> > > > +
> > > > +	if (req->u.socket.domain != AF_INET ||
> > > > +	    req->u.socket.type != SOCK_STREAM ||
> > > > +	    (req->u.socket.protocol != IPPROTO_IP &&
> > > > +	     req->u.socket.protocol != AF_INET))
> > > > +		ret = -EAFNOSUPPORT;
> > > 
> > > Sorry for jumping into this out of the blue, but shouldn't all the
> > > constants used above be part of the protocol? AF_INET/SOCK_STREAM/...
> > > are all part of POSIX, but their specific value is not defined in the
> > > standard, hence we should have XEN_AF_INET/XEN_SOCK_STREAM/... Or am I
> > > just missing something?
> > 
> > The values of these constants for the pvcalls protocol are defined by
> > docs/misc/pvcalls.markdown under "Socket families and address format".
> > 
> > They happen to be the same as the ones defined by Linux as AF_INET,
> > SOCK_STREAM, etc, so in Linux I am just using those, but that is just an
> > implementation detail internal to the Linux kernel driver. What is
> > important from the protocol ABI perspective are the values defined by
> > docs/misc/pvcalls.markdown.
> 
> Oh I see. I still think this should be part of the public pvcalls.h
> header, and that the error codes should be the ones defined in
> public/errno.h (or else also added to the pvcalls header).

This was done differently in the past, but now that we have a formal
process, a person in charge of new PV drivers reviews, and design
documents with clearly spelled out ABIs, I consider the design docs
under docs/misc as the official specification. We don't need headers
anymore, they are redundant. In fact, we cannot have two specifications,
and the design docs are certainly the official ones (we don't want the
specs to be written as header files in C). To me, the headers under
xen/include/public/io/ are optional helpers. It doesn't matter what's in
there, or if frontends and backends use them or not.

There is really an argument for removing those headers, because they
might get out of sync with the spec by mistake, and in those cases, then
we really end up with two specifications for the same protocol. I would
be in favor of `git rm'ing all files under xen/include/public/io/ for
which we have a complete design doc under docs/misc.
--8323329-1888458059-1498156185=:12819--