Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1670927 > unrolled thread

Re: [Xen-devel] [PATCH v4 07/18] xen/pvcalls: implement socket command

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [Xen-devel] [PATCH v4 07/18] xen/pvcalls: implement socket  command Roger Pau Monné <roger.pau@citrix.com> - 2017-06-20 18:20 +0200
    Re: [Xen-devel] [PATCH v4 07/18] xen/pvcalls: implement socket  command Stefano Stabellini <sstabellini@kernel.org> - 2017-06-21 22:20 +0200
      Re: [Xen-devel] [PATCH v4 07/18] xen/pvcalls: implement socket  command Roger Pau Monné <roger.pau@citrix.com> - 2017-06-22 10:30 +0200
        Re: [Xen-devel] [PATCH v4 07/18] xen/pvcalls: implement socket  command Stefano Stabellini <sstabellini@kernel.org> - 2017-06-22 20:30 +0200
          Re: [Xen-devel] [PATCH v4 07/18] xen/pvcalls: implement socket  command Andrew Cooper <andrew.cooper3@citrix.com> - 2017-06-22 20:40 +0200
            Re: [Xen-devel] [PATCH v4 07/18] xen/pvcalls: implement socket  command Stefano Stabellini <sstabellini@kernel.org> - 2017-06-22 20:40 +0200
          Re: [Xen-devel] [PATCH v4 07/18] xen/pvcalls: implement socket  command Roger Pau Monné <roger.pau@citrix.com> - 2017-06-23 11:40 +0200

#1670927 — Re: [Xen-devel] [PATCH v4 07/18] xen/pvcalls: implement socket command

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?

Roger.

[toc] | [next] | [standalone]

#1672005

[Multipart message — attachments visible in raw view] — view raw

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.

[toc] | [prev] | [next] | [standalone]

#1672403

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).

Roger.

[toc] | [prev] | [next] | [standalone]

#1672942

[Multipart message — attachments visible in raw view] — view raw

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.

[toc] | [prev] | [next] | [standalone]

#1672951

On 22/06/17 19:29, Stefano Stabellini wrote:
> 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.

+1.

Specifications should not be written in C.  The mess that is the net and
block protocol ABIs are perfect examples of why.

Its fine (and indeed recommended) to provide a header file which
describes the specified protocol, but the authoritative spec should be
in text from.

I would really prefer if more people started using ../docs/specs/.  The
migration v2 documents are currently lonely there...

~Andrew

[toc] | [prev] | [next] | [standalone]

#1672952

[Multipart message — attachments visible in raw view] — view raw

On Thu, 22 Jun 2017, Andrew Cooper wrote:
> On 22/06/17 19:29, Stefano Stabellini wrote:
> > 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.
> 
> +1.
> 
> Specifications should not be written in C.  The mess that is the net and
> block protocol ABIs are perfect examples of why.
> 
> Its fine (and indeed recommended) to provide a header file which
> describes the specified protocol, but the authoritative spec should be
> in text from.
> 
> I would really prefer if more people started using ../docs/specs/.  The
> migration v2 documents are currently lonely there...

I didn't realize we had a docs/specs. Feel free to move pvcalls and 9pfs
under there.

[toc] | [prev] | [next] | [standalone]

#1673438

On Thu, Jun 22, 2017 at 11:29:44AM -0700, Stefano Stabellini wrote:
> 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.

Thanks for the clarification. I agree that it would be good to remove
those headers, it's confusing and it's likely that they will get out
of sync.

Roger.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web