[Xenomai-help] message pipe message boundaries

[Xenomai-help] message pipe message boundaries

[dietmar.schindler]

The documentation of rt_pipe_write() says: "rt_pipe_write() always
preserves message boundaries, which means that all data sent through a
single call of this service will be gathered in a single read(2)
operation from the special device." Is this supposed to mean also that
data sent through two calls of this service will be gathered in two
read(2) operations, or would it be conforming that data sent through two
calls of this service will be gathered in a single read(2) operation?

--------------------------------------------------------
manroland AG
Vorsitzender des Aufsichtsrates: Hanno C. Fiedler
Vorstand: Gerd Finkbeiner (Vorsitzender), Dr. Ingo Koch, Dr. Markus Rall, Paul Steidle   
Sitz der Gesellschaft: Offenbach am Main, Registergericht: Amtsgericht Offenbach HRB-Nr. 42592
USt-Ident-Nr. DE 250200933

Re: [Xenomai-help] message pipe message boundaries

[Philippe Gerum]

On Tue, 2011-04-26 at 11:48 +0200, dietmar.schindler@domain.hid
wrote:
> The documentation of rt_pipe_write() says: "rt_pipe_write() always
> preserves message boundaries, which means that all data sent through a
> single call of this service will be gathered in a single read(2)
> operation from the special device." Is this supposed to mean also that
> data sent through two calls of this service will be gathered in two
> read(2) operations, or would it be conforming that data sent through two
> calls of this service will be gathered in a single read(2) operation?

One read() per rt_pipe_write() operation, always.

> 
> --------------------------------------------------------
> manroland AG
> Vorsitzender des Aufsichtsrates: Hanno C. Fiedler
> Vorstand: Gerd Finkbeiner (Vorsitzender), Dr. Ingo Koch, Dr. Markus Rall, Paul Steidle   
> Sitz der Gesellschaft: Offenbach am Main, Registergericht: Amtsgericht Offenbach HRB-Nr. 42592
> USt-Ident-Nr. DE 250200933
> 
> _______________________________________________
> Xenomai-help mailing list
> Xenomai-help@domain.hid
> https://mail.gna.org/listinfo/xenomai-help

-- 
Philippe.

Re: [Xenomai-help] message pipe message boundaries

[dietmar.schindler]

> From: xenomai-help-bounces@domain.hid
[mailto:xenomai-help-bounces@domain.hid]
> On Behalf Of Philippe Gerum
> Sent: Tuesday, April 26, 2011 10:28 PM
> 
> On Tue, 2011-04-26 at 11:48 +0200, dietmar.schindler@domain.hid
> wrote:
> > The documentation of rt_pipe_write() says: "rt_pipe_write() always
> > preserves message boundaries, which means that all data sent through
a
> > single call of this service will be gathered in a single read(2)
> > operation from the special device." Is this supposed to mean also
that
> > data sent through two calls of this service will be gathered in two
> > read(2) operations, or would it be conforming that data sent through
two
> > calls of this service will be gathered in a single read(2)
operation?
> 
> One read() per rt_pipe_write() operation, always.

Thank you for your answer. Is "One readv() per rt_pipe_write()
operation" supposed to be true also, or is there something special to be
taken into consideration regarding readv(2)?

-- 
Dietmar

--------------------------------------------------------
manroland AG
Vorsitzender des Aufsichtsrates: Hanno C. Fiedler
Vorstand: Gerd Finkbeiner (Vorsitzender), Dr. Ingo Koch, Dr. Markus Rall, Paul Steidle   
Sitz der Gesellschaft: Offenbach am Main, Registergericht: Amtsgericht Offenbach HRB-Nr. 42592
USt-Ident-Nr. DE 250200933

Re: [Xenomai-help] message pipe message boundaries

[dietmar.schindler]

> From: xenomai-help-bounces@domain.hid
[mailto:xenomai-help-bounces@domain.hid]
> On Behalf Of dietmar.schindler@domain.hid
> Sent: Wednesday, April 27, 2011 10:50 AM
> 
> > From: xenomai-help-bounces@domain.hid
> > On Behalf Of Philippe Gerum
> > Sent: Tuesday, April 26, 2011 10:28 PM
> >
> > ...
> > One read() per rt_pipe_write() operation, always.
> 
> Thank you for your answer. Is "One readv() per rt_pipe_write()
> operation" supposed to be true also, or is there something special to
be
> taken into consideration regarding readv(2)?

Meanwhile, I found some facts which allow me to elaborate on my
question. The rtpipe device hasn't a readv file operation implemented,
so the kernel substitutes the readv call by a series of read calls to
the device driver; due to this, under certain circumstances readv(2)
gathers more than a single rt_pipe_write() call's data. (I can provide
an example case, if desired.) Wouldn't it be desirable to implement the
readv file operation in the rtpipe driver so that the guarantee "One
read() per rt_pipe_write()" could cover readv(2) also? (Besides, this
would make the phrase "Operates just like read" in the readv(2) man page
more correct.)
-- 
Dietmar

--------------------------------------------------------
manroland AG
Vorsitzender des Aufsichtsrates: Hanno C. Fiedler
Vorstand: Gerd Finkbeiner (Vorsitzender), Dr. Ingo Koch, Dr. Markus Rall, Paul Steidle   
Sitz der Gesellschaft: Offenbach am Main, Registergericht: Amtsgericht Offenbach HRB-Nr. 42592
USt-Ident-Nr. DE 250200933

Re: [Xenomai-help] message pipe message boundaries

[Philippe Gerum]

On Wed, 2011-04-27 at 14:22 +0200, dietmar.schindler@domain.hid
wrote:
> > From: xenomai-help-bounces@domain.hid
> [mailto:xenomai-help-bounces@domain.hid]
> > On Behalf Of dietmar.schindler@domain.hid
> > Sent: Wednesday, April 27, 2011 10:50 AM
> > 
> > > From: xenomai-help-bounces@domain.hid
> > > On Behalf Of Philippe Gerum
> > > Sent: Tuesday, April 26, 2011 10:28 PM
> > >
> > > ...
> > > One read() per rt_pipe_write() operation, always.
> > 
> > Thank you for your answer. Is "One readv() per rt_pipe_write()
> > operation" supposed to be true also, or is there something special to
> be
> > taken into consideration regarding readv(2)?
> 
> Meanwhile, I found some facts which allow me to elaborate on my
> question. The rtpipe device hasn't a readv file operation implemented,
> so the kernel substitutes the readv call by a series of read calls to
> the device driver; due to this, under certain circumstances readv(2)
> gathers more than a single rt_pipe_write() call's data. (I can provide
> an example case, if desired.) Wouldn't it be desirable to implement the
> readv file operation in the rtpipe driver so that the guarantee "One
> read() per rt_pipe_write()" could cover readv(2) also?

No objection to merge this. Patch welcome.

>  (Besides, this
> would make the phrase "Operates just like read" in the readv(2) man page
> more correct.)

Having readv() operate like read() would not give any guarantee wrt
message boundaries, because it is the real-time write side which makes
the decision regarding this, not the read side. So you could have
readv() operate like read() and still have blurred boundaries in case
rt_pipe_stream() is used instead of rt_pipe_write().

-- 
Philippe.

Re: [Xenomai-help] message pipe message boundaries

[dietmar.schindler]

> From: Philippe Gerum [mailto:rpm@xenomai.org]
> Sent: Wednesday, April 27, 2011 11:43 PM
> 
> On Wed, 2011-04-27 at 14:22 +0200, dietmar.schindler@domain.hid
> wrote:
> > ... Wouldn't it be desirable to implement the
> > readv file operation in the rtpipe driver so that the guarantee "One
> > read() per rt_pipe_write()" could cover readv(2) also?
> 
> No objection to merge this. Patch welcome.

Well, I'll try to come up with one.

> >  (Besides, this
> > would make the phrase "Operates just like read" in the readv(2) man page
> > more correct.)
> 
> Having readv() operate like read() would not give any guarantee wrt
> message boundaries, because it is the real-time write side which makes
> the decision regarding this, not the read side. So you could have
> readv() operate like read() and still have blurred boundaries in case
> rt_pipe_stream() is used instead of rt_pipe_write().

You are of course right, and what you wrote about readv also applies to read.

-- 
Dietmar

--------------------------------------------------------
manroland AG
Vorsitzender des Aufsichtsrates: Hanno C. Fiedler
Vorstand: Gerd Finkbeiner (Vorsitzender), Dr. Ingo Koch, Dr. Markus Rall, Paul Steidle   
Sitz der Gesellschaft: Offenbach am Main, Registergericht: Amtsgericht Offenbach HRB-Nr. 42592
USt-Ident-Nr. DE 250200933

Re: [Xenomai-help] message pipe message boundaries

[dietmar.schindler]

[-- Attachment #1: Type: text/plain, Size: 1327 bytes --]

> From: xenomai-help-bounces@domain.hid
[mailto:xenomai-help-bounces@domain.hid]
> On Behalf Of dietmar.schindler@domain.hid
> Sent: Friday, April 29, 2011 8:31 AM
> Cc: xenomai@xenomai.org
> Subject: Re: [Xenomai-help] message pipe message boundaries
> 
> > From: Philippe Gerum [mailto:rpm@xenomai.org]
> > Sent: Wednesday, April 27, 2011 11:43 PM
> >
> > On Wed, 2011-04-27 at 14:22 +0200, dietmar.schindler@domain.hid
> > wrote:
> > > ... Wouldn't it be desirable to implement the
> > > readv file operation in the rtpipe driver so that the guarantee
"One
> > > read() per rt_pipe_write()" could cover readv(2) also?
> >
> > No objection to merge this. Patch welcome.
> 
> Well, I'll try to come up with one.

We at manroland now use (with Xenomai 2.4) the attached patch, which
implements the readv file operation in the rtpipe driver so that the
assertion "One read() per rt_pipe_write()" (message boundaries
preserved) covers readv(2) also.
-- 
Dietmar

--------------------------------------------------------
manroland AG
Vorsitzender des Aufsichtsrates: Hanno C. Fiedler
Vorstand: Gerd Finkbeiner (Vorsitzender), Dr. Ingo Koch, Dr. Markus Rall, Paul Steidle   
Sitz der Gesellschaft: Offenbach am Main, Registergericht: Amtsgericht Offenbach HRB-Nr. 42592
USt-Ident-Nr. DE 250200933

[-- Attachment #2: pipe.diff --]
[-- Type: application/octet-stream, Size: 2314 bytes --]

Index: branches/MR_Xenomai2_4/source/ksrc/nucleus/pipe.c
===================================================================
--- branches/MR_Xenomai2_4/source/ksrc/nucleus/pipe.c	(revision 3555)
+++ branches/MR_Xenomai2_4/source/ksrc/nucleus/pipe.c	(revision 3556)
@@ -729,8 +729,8 @@
 	return 0;
 }
 
-static ssize_t xnpipe_read(struct file *file,
-			   char *buf, size_t count, loff_t *ppos)
+static ssize_t xnpipe_read_v_(struct file *file, void *buf_iovec, size_t count,
+			      int v)
 {
 	struct xnpipe_state *state = file->private_data;
 	int sigpending, err = 0;
@@ -739,9 +739,12 @@
 	struct xnholder *h;
 	ssize_t ret;
 	spl_t s;
+	char *buf;
+	size_t vec_count;
+	const struct iovec *vec;
 
-	if (!access_ok(VERIFY_WRITE, buf, count))
-		return -EFAULT;
+	if (v)	vec = buf_iovec, vec_count = count, count = 0;
+	else	buf = buf_iovec;
 
 	xnlock_get_irqsave(&nklock, s);
 
@@ -781,9 +784,20 @@
 	 * entirely.
 	 */
 
+	ret =
 	inbytes = 0;
 
 	for (;;) {
+		if (v) if (inbytes == count) if (vec_count) {
+		    buf = vec->iov_base, count = vec++->iov_len, --vec_count;
+		    inbytes = 0;
+		    if (!access_ok(VERIFY_WRITE, buf, count)) {
+			err = -EFAULT; break;
+		    }
+
+		    continue;
+		}
+
 		nbytes = xnpipe_m_size(mh) - xnpipe_m_rdoff(mh);
 
 		if (nbytes + inbytes > count)
@@ -806,11 +820,11 @@
 		}
 
 		inbytes += nbytes;
+		ret += nbytes;
 		xnpipe_m_rdoff(mh) += nbytes;
 	}
 
-	state->ionrd -= inbytes;
-	ret = inbytes;
+	state->ionrd -= ret;
 
 	if (xnpipe_m_size(mh) > xnpipe_m_rdoff(mh))
 		prependq(&state->outq, &mh->link);
@@ -837,6 +851,21 @@
 	return err ? : ret;
 }
 
+static ssize_t xnpipe_read(struct file *file,
+			   char *buf, size_t count, loff_t *ppos)
+{
+	if (!access_ok(VERIFY_WRITE, buf, count))
+		return -EFAULT;
+
+	return xnpipe_read_v_(file, buf, count, 0);
+}
+
+static ssize_t xnpipe_readv(struct file *file, const struct iovec *iovec,
+			    unsigned long count, loff_t *ppos)
+{
+	return xnpipe_read_v_(file, iovec, count, 1);
+}
+
 static ssize_t xnpipe_write(struct file *file,
 			    const char *buf, size_t count, loff_t *ppos)
 {
@@ -1060,6 +1089,7 @@
 static struct file_operations xnpipe_fops = {
 	.owner = THIS_MODULE,
 	.read = xnpipe_read,
+	.readv = xnpipe_readv,
 	.write = xnpipe_write,
 	.poll = xnpipe_poll,
 	.ioctl = xnpipe_ioctl,

Re: [Xenomai-help] message pipe message boundaries

[Gilles Chanteperdrix]

On 05/03/2011 10:15 AM, dietmar.schindler@domain.hid wrote:
>> From: xenomai-help-bounces@domain.hid
> [mailto:xenomai-help-bounces@domain.hid]
>> On Behalf Of dietmar.schindler@domain.hid
>> Sent: Friday, April 29, 2011 8:31 AM
>> Cc: xenomai@xenomai.org
>> Subject: Re: [Xenomai-help] message pipe message boundaries
>>
>>> From: Philippe Gerum [mailto:rpm@xenomai.org]
>>> Sent: Wednesday, April 27, 2011 11:43 PM
>>>
>>> On Wed, 2011-04-27 at 14:22 +0200, dietmar.schindler@domain.hid
>>> wrote:
>>>> ... Wouldn't it be desirable to implement the
>>>> readv file operation in the rtpipe driver so that the guarantee
> "One
>>>> read() per rt_pipe_write()" could cover readv(2) also?
>>>
>>> No objection to merge this. Patch welcome.
>>
>> Well, I'll try to come up with one.
> 
> We at manroland now use (with Xenomai 2.4) the attached patch, which
> implements the readv file operation in the rtpipe driver so that the
> assertion "One read() per rt_pipe_write()" (message boundaries
> preserved) covers readv(2) also.
> 

I think implementing the readv method is sufficient, you do not need to
implement both readv and read.

-- 
                                                                Gilles.

Re: [Xenomai-help] message pipe message boundaries

[dietmar.schindler]

> From: xenomai-help-bounces@domain.hid
[mailto:xenomai-help-bounces@domain.hid]
> On Behalf Of Gilles Chanteperdrix
> Sent: Tuesday, May 03, 2011 1:01 PM
> 
> On 05/03/2011 10:15 AM, dietmar.schindler@domain.hid wrote:
> >...
> > We at manroland now use (with Xenomai 2.4) the attached patch, which
> > implements the readv file operation in the rtpipe driver so that the
> > assertion "One read() per rt_pipe_write()" (message boundaries
> > preserved) covers readv(2) also.
> >
> 
> I think implementing the readv method is sufficient, you do not need
to
> implement both readv and read.

I agree; since read was already implemented, I just extended it to
support readv.
-- 
Dietmar

--------------------------------------------------------
manroland AG
Vorsitzender des Aufsichtsrates: Hanno C. Fiedler
Vorstand: Gerd Finkbeiner (Vorsitzender), Dr. Ingo Koch, Dr. Markus Rall, Paul Steidle   
Sitz der Gesellschaft: Offenbach am Main, Registergericht: Amtsgericht Offenbach HRB-Nr. 42592
USt-Ident-Nr. DE 250200933

Re: [Xenomai-help] message pipe message boundaries

[Gilles Chanteperdrix]

On 05/03/2011 01:27 PM, dietmar.schindler@domain.hid wrote:
>> From: xenomai-help-bounces@domain.hid
> [mailto:xenomai-help-bounces@domain.hid]
>> On Behalf Of Gilles Chanteperdrix
>> Sent: Tuesday, May 03, 2011 1:01 PM
>>
>> On 05/03/2011 10:15 AM, dietmar.schindler@domain.hid wrote:
>>> ...
>>> We at manroland now use (with Xenomai 2.4) the attached patch, which
>>> implements the readv file operation in the rtpipe driver so that the
>>> assertion "One read() per rt_pipe_write()" (message boundaries
>>> preserved) covers readv(2) also.
>>>
>>
>> I think implementing the readv method is sufficient, you do not need
> to
>> implement both readv and read.
> 
> I agree; since read was already implemented, I just extended it to
> support readv.

Unfortunately, the patch as is is a bit hard to read. IMO, only
implementing readv will result in a more straight-forward implementation
which we could merge.

-- 
                                                                Gilles.

Re: [Xenomai-help] message pipe message boundaries

[dietmar.schindler]

> From: Gilles Chanteperdrix [mailto:gilles.chanteperdrix@xenomai.org]
> Sent: Tuesday, May 03, 2011 7:36 PM
> ...
> >> On 05/03/2011 10:15 AM, dietmar.schindler@domain.hid wrote:
> >>> ...
> >>> We at manroland now use (with Xenomai 2.4) the attached patch, which
> >>> implements the readv file operation in the rtpipe driver so that the
> >>> assertion "One read() per rt_pipe_write()" (message boundaries
> >>> preserved) covers readv(2) also.
> ...
> Unfortunately, the patch as is is a bit hard to read. IMO, only
> implementing readv will result in a more straight-forward implementation
> which we could merge.

I understand. If you prefer, I can provide a patch which multiplies out the functions to a separate readv, though this will result in code duplication of the whole read function. Alternatively, I could comment the changes so that they are easier to read.
Please let me know which flavour you favour.
-- 
Dietmar

--------------------------------------------------------
manroland AG
Vorsitzender des Aufsichtsrates: Hanno C. Fiedler
Vorstand: Gerd Finkbeiner (Vorsitzender), Dr. Ingo Koch, Dr. Markus Rall, Paul Steidle   
Sitz der Gesellschaft: Offenbach am Main, Registergericht: Amtsgericht Offenbach HRB-Nr. 42592
USt-Ident-Nr. DE 250200933

Re: [Xenomai-help] message pipe message boundaries

[Gilles Chanteperdrix]

On 05/04/2011 08:36 AM, dietmar.schindler@domain.hid wrote:
>> From: Gilles Chanteperdrix [mailto:gilles.chanteperdrix@xenomai.org]
>> Sent: Tuesday, May 03, 2011 7:36 PM
>> ...
>>>> On 05/03/2011 10:15 AM, dietmar.schindler@domain.hid wrote:
>>>>> ...
>>>>> We at manroland now use (with Xenomai 2.4) the attached patch, which
>>>>> implements the readv file operation in the rtpipe driver so that the
>>>>> assertion "One read() per rt_pipe_write()" (message boundaries
>>>>> preserved) covers readv(2) also.
>> ...
>> Unfortunately, the patch as is is a bit hard to read. IMO, only
>> implementing readv will result in a more straight-forward implementation
>> which we could merge.
> 
> I understand. If you prefer, I can provide a patch which multiplies out the functions to a separate readv, though this will result in code duplication of the whole read function. Alternatively, I could comment the changes so that they are easier to read.
> Please let me know which flavour you favour.

What I suggest, is that you make a patch which removes the read
implementation and adds the readv implementation. In this case, the
kernel will use readv to implement read anyway, and there will be no
code duplication.

-- 
                                                                Gilles.

Re: [Xenomai-help] message pipe message boundaries

[dietmar.schindler]

> From: Gilles Chanteperdrix [mailto:gilles.chanteperdrix@xenomai.org]
> Sent: Wednesday, May 04, 2011 8:41 AM
> 
> What I suggest, is that you make a patch which removes the read
> implementation and adds the readv implementation. In this case, the
> kernel will use readv to implement read anyway, and there will be no
> code duplication.

Thank you for the clarification. However, in the (still used by us) Linux 2.4.25 kernel's function

asmlinkage ssize_t sys_read(unsigned int fd, char * buf, size_t count)
{
    ssize_t ret;
    struct file * file;

    ret = -EBADF;
    file = fget(fd);
    if (file) {
        if (file->f_mode & FMODE_READ) {
            ret = locks_verify_area(FLOCK_VERIFY_READ, file->f_dentry->d_inode,
                        file, file->f_pos, count);
            if (!ret) {
                ssize_t (*read)(struct file *, char *, size_t, loff_t *);
                ret = -EINVAL;
                if (file->f_op && (read = file->f_op->read) != NULL)
                    ret = read(file, buf, count, &file->f_pos);
            }
        }
        if (ret > 0)
            dnotify_parent(file->f_dentry, DN_ACCESS);
        fput(file);
    }
    return ret;
}

it doesn’t look like it would use readv to implement read, so I'm afraid that your otherwise good suggestion would not work with such old kernel versions.

-- 
Dietmar

--------------------------------------------------------
manroland AG
Vorsitzender des Aufsichtsrates: Hanno C. Fiedler
Vorstand: Gerd Finkbeiner (Vorsitzender), Dr. Ingo Koch, Dr. Markus Rall, Paul Steidle   
Sitz der Gesellschaft: Offenbach am Main, Registergericht: Amtsgericht Offenbach HRB-Nr. 42592
USt-Ident-Nr. DE 250200933

Re: [Xenomai-help] message pipe message boundaries

[Gilles Chanteperdrix]

On 05/04/2011 04:34 PM, dietmar.schindler@domain.hid wrote:
>> From: Gilles Chanteperdrix
>> [mailto:gilles.chanteperdrix@xenomai.org] Sent: Wednesday, May 04,
>> 2011 8:41 AM
>> 
>> What I suggest, is that you make a patch which removes the read 
>> implementation and adds the readv implementation. In this case,
>> the kernel will use readv to implement read anyway, and there will
>> be no code duplication.
> 
> Thank you for the clarification. However, in the (still used by us)
> Linux 2.4.25 kernel's function
> 
> asmlinkage ssize_t sys_read(unsigned int fd, char * buf, size_t
> count) { ssize_t ret; struct file * file;
> 
> ret = -EBADF; file = fget(fd); if (file) { if (file->f_mode &
> FMODE_READ) { ret = locks_verify_area(FLOCK_VERIFY_READ,
> file->f_dentry->d_inode, file, file->f_pos, count); if (!ret) { 
> ssize_t (*read)(struct file *, char *, size_t, loff_t *); ret =
> -EINVAL; if (file->f_op && (read = file->f_op->read) != NULL) ret =
> read(file, buf, count, &file->f_pos); } } if (ret > 0) 
> dnotify_parent(file->f_dentry, DN_ACCESS); fput(file); } return ret; 
> }
> 
> it doesn’t look like it would use readv to implement read, so I'm
> afraid that your otherwise good suggestion would not work with such
> old kernel versions.

Hi,

You can provide the readv implementation, and implement read in terms of
readv by passing an iovec with just one buffer. Please also try and make
the code a bit more readable than in the previous patch, with only one
statement and one "if" by line.

Thanks in advance.

-- 
					    Gilles.

Re: [Xenomai-help] message pipe message boundaries

[dietmar.schindler]

[-- Attachment #1: Type: text/plain, Size: 883 bytes --]

> From: Gilles Chanteperdrix [mailto:gilles.chanteperdrix@xenomai.org]
> Sent: Thursday, May 05, 2011 10:15 AM
> ...
> You can provide the readv implementation, and implement read in terms of
> readv by passing an iovec with just one buffer. Please also try and make
> the code a bit more readable than in the previous patch, with only one
> statement and one "if" by line.

Hello,

thank you for your suggestions. I now remade the patch accordingly. If you have further improvements to suggest, please let me know.

-- 
Dietmar

--------------------------------------------------------
manroland AG
Vorsitzender des Aufsichtsrates: Hanno C. Fiedler
Vorstand: Gerd Finkbeiner (Vorsitzender), Dr. Ingo Koch, Dr. Markus Rall, Paul Steidle   
Sitz der Gesellschaft: Offenbach am Main, Registergericht: Amtsgericht Offenbach HRB-Nr. 42592
USt-Ident-Nr. DE 250200933

[-- Attachment #2: pipe.diff --]
[-- Type: application/octet-stream, Size: 1887 bytes --]

Index: pipe.c
===================================================================
--- pipe.c	(revision 3555)
+++ pipe.c	(revision 3577)
@@ -729,8 +729,8 @@
 	return 0;
 }
 
-static ssize_t xnpipe_read(struct file *file,
-			   char *buf, size_t count, loff_t *ppos)
+static ssize_t xnpipe_readv(struct file *file, const struct iovec *iovec,
+			    unsigned long nr_segs, loff_t *ppos)
 {
 	struct xnpipe_state *state = file->private_data;
 	int sigpending, err = 0;
@@ -739,9 +739,8 @@
 	struct xnholder *h;
 	ssize_t ret;
 	spl_t s;
-
-	if (!access_ok(VERIFY_WRITE, buf, count))
-		return -EFAULT;
+	char *buf;
+	size_t count = 0;
 
 	xnlock_get_irqsave(&nklock, s);
 
@@ -782,8 +781,21 @@
 	 */
 
 	inbytes = 0;
+	ret = 0;
 
 	for (;;) {
+		if (inbytes == count && nr_segs) {
+			buf = iovec->iov_base, count = iovec++->iov_len;
+			--nr_segs;
+			inbytes = 0;
+			if (!access_ok(VERIFY_WRITE, buf, count)) {
+				err = -EFAULT;
+				break;
+			}
+
+			continue;
+		}
+
 		nbytes = xnpipe_m_size(mh) - xnpipe_m_rdoff(mh);
 
 		if (nbytes + inbytes > count)
@@ -807,10 +819,10 @@
 
 		inbytes += nbytes;
 		xnpipe_m_rdoff(mh) += nbytes;
+		ret += nbytes;
 	}
 
-	state->ionrd -= inbytes;
-	ret = inbytes;
+	state->ionrd -= ret;
 
 	if (xnpipe_m_size(mh) > xnpipe_m_rdoff(mh))
 		prependq(&state->outq, &mh->link);
@@ -837,6 +849,13 @@
 	return err ? : ret;
 }
 
+static ssize_t xnpipe_read(struct file *file,
+			   char *buf, size_t count, loff_t *ppos)
+{
+	struct iovec iov = { .iov_base = buf, .iov_len = count };
+	return xnpipe_readv(file, &iov, 1, ppos);
+}
+
 static ssize_t xnpipe_write(struct file *file,
 			    const char *buf, size_t count, loff_t *ppos)
 {
@@ -1060,6 +1079,7 @@
 static struct file_operations xnpipe_fops = {
 	.owner = THIS_MODULE,
 	.read = xnpipe_read,
+	.readv = xnpipe_readv,
 	.write = xnpipe_write,
 	.poll = xnpipe_poll,
 	.ioctl = xnpipe_ioctl,

Re: [Xenomai-help] message pipe message boundaries

[Gilles Chanteperdrix]

On 05/09/2011 09:36 AM, dietmar.schindler@domain.hid wrote:
>> From: Gilles Chanteperdrix
>> [mailto:gilles.chanteperdrix@xenomai.org] Sent: Thursday, May 05,
>> 2011 10:15 AM ... You can provide the readv implementation, and
>> implement read in terms of readv by passing an iovec with just one
>> buffer. Please also try and make the code a bit more readable than
>> in the previous patch, with only one statement and one "if" by
>> line.
> 
> Hello,
> 
> thank you for your suggestions. I now remade the patch accordingly.
> If you have further improvements to suggest, please let me know.

I am sorry to ask you to change your patch again, but the loop is quite
hard to understand. It would be nice if you could use a simple loops
which copies the pieces of the iovec. So, maybe use two nested loops.

-- 
					    Gilles.

Re: [Xenomai-help] message pipe message boundaries

[Philippe Gerum]

On Wed, 2011-05-11 at 11:12 +0200, Gilles Chanteperdrix wrote:
> On 05/09/2011 09:36 AM, dietmar.schindler@domain.hid wrote:
> >> From: Gilles Chanteperdrix
> >> [mailto:gilles.chanteperdrix@xenomai.org] Sent: Thursday, May 05,
> >> 2011 10:15 AM ... You can provide the readv implementation, and
> >> implement read in terms of readv by passing an iovec with just one
> >> buffer. Please also try and make the code a bit more readable than
> >> in the previous patch, with only one statement and one "if" by
> >> line.
> > 
> > Hello,
> > 
> > thank you for your suggestions. I now remade the patch accordingly.
> > If you have further improvements to suggest, please let me know.
> 
> I am sorry to ask you to change your patch again, but the loop is quite
> hard to understand. It would be nice if you could use a simple loops
> which copies the pieces of the iovec. So, maybe use two nested loops.
> 


Also, let's avoid constructs like these:

>                       buf = iovec->iov_base, count = iovec++->iov_len;

Disk space is cheap so we can afford a few more characters per line;
however, engineering time for tracking down issues introduced by
obfuscated code is expensive.

In short, one executable statement per line is desired, our code should
have nothing to hide.

-- 
Philippe.

Re: [Xenomai-help] message pipe message boundaries

[dietmar.schindler]

> From: Philippe Gerum [mailto:rpm@xenomai.org]
> Sent: Wednesday, May 11, 2011 11:21 AM
> ...
> Also, let's avoid constructs like these:
> 
> >                       buf = iovec->iov_base, count = iovec++->iov_len;
> 
> Disk space is cheap so we can afford a few more characters per line;
> ...

Could you please explain what you mean by this? I can imagine to put a few more characters into the line above by changing it to

	buf = iovec->iov_base, count = iovec->iov_len, ++iovec;

but this would seem to contradict your recommendation below.

> In short, one executable statement per line is desired, our code should
> have nothing to hide.

Since the above code line contains only one statement, I assume you mean one assignment-expression, and will do accordingly.

By the way, what is your convention regarding goto?
-- 
Dietmar

--------------------------------------------------------
manroland AG
Vorsitzender des Aufsichtsrates: Hanno C. Fiedler
Vorstand: Gerd Finkbeiner (Vorsitzender), Dr. Ingo Koch, Dr. Markus Rall, Paul Steidle   
Sitz der Gesellschaft: Offenbach am Main, Registergericht: Amtsgericht Offenbach HRB-Nr. 42592
USt-Ident-Nr. DE 250200933

Re: [Xenomai-help] message pipe message boundaries

[Gilles Chanteperdrix]

On 05/11/2011 12:48 PM, dietmar.schindler@domain.hid wrote:
>> From: Philippe Gerum [mailto:rpm@xenomai.org]
>> Sent: Wednesday, May 11, 2011 11:21 AM
>> ...
>> Also, let's avoid constructs like these:
>>
>>>                       buf = iovec->iov_base, count = iovec++->iov_len;
>>
>> Disk space is cheap so we can afford a few more characters per line;
>> ...
> 
> Could you please explain what you mean by this? I can imagine to put a few more characters into the line above by changing it to
> 
> 	buf = iovec->iov_base, count = iovec->iov_len, ++iovec;
> 
> but this would seem to contradict your recommendation below.
> 
>> In short, one executable statement per line is desired, our code should
>> have nothing to hide.
> 
> Since the above code line contains only one statement, I assume you mean one assignment-expression, and will do accordingly.
> 
> By the way, what is your convention regarding goto?

goto is allowed, but use it only when needed. Meaning:
- if there is a way to arrange the control flow without goto and without
resorting to boolean flags variables, or to code duplication, do it, do
not use goto;
- otherwise use goto, choosing a readable label, as the only thing which
makes goto readable is the label you choose.

-- 
                                                                Gilles.

Re: [Xenomai-help] message pipe message boundaries

[Philippe Gerum]

On Wed, 2011-05-11 at 12:48 +0200, dietmar.schindler@domain.hid
wrote:
> > From: Philippe Gerum [mailto:rpm@xenomai.org]
> > Sent: Wednesday, May 11, 2011 11:21 AM
> > ...
> > Also, let's avoid constructs like these:
> > 
> > >                       buf = iovec->iov_base, count = iovec++->iov_len;
> > 
> > Disk space is cheap so we can afford a few more characters per line;
> > ...
> 
> Could you please explain what you mean by this? I can imagine to put a few more characters into the line above by changing it to
> 
> 	buf = iovec->iov_base, count = iovec->iov_len, ++iovec;
> 
> but this would seem to contradict your recommendation below.

	buf = iovec->iov_base;
	count = iovec->iov_len;
 	++iovec;

One exec statement per line, no comma operator, no combined
post-incrementation and dereference to obfuscate the obvious, because we
don't care about a few more characters in the source code: we do want
readability. Plain, silly, massively non-imaginative obviousness of
constructs.

> 
> > In short, one executable statement per line is desired, our code should
> > have nothing to hide.
> 
> Since the above code line contains only one statement, I assume you mean one assignment-expression, and will do accordingly.
> 
> By the way, what is your convention regarding goto?

Absolutely fine to use it for handling error paths sanely without code
duplication, and anywhere it would spare us weird games based on
obfuscated variable updates and tests only to control the code flow.

-- 
Philippe.

Re: [Xenomai-help] message pipe message boundaries

[dietmar.schindler]

[-- Attachment #1: Type: text/plain, Size: 818 bytes --]

> From: Gilles Chanteperdrix [mailto:gilles.chanteperdrix@xenomai.org]
> Sent: Wednesday, May 11, 2011 11:12 AM
> ...
> I am sorry to ask you to change your patch again, but the loop is quite
> hard to understand. It would be nice if you could use a simple loops
> which copies the pieces of the iovec. So, maybe use two nested loops.

Never mind. I now remade the patch with two nested loops. If you have further changes to suggest, please let me know.

-- 
Dietmar

--------------------------------------------------------
manroland AG
Vorsitzender des Aufsichtsrates: Hanno C. Fiedler
Vorstand: Gerd Finkbeiner (Vorsitzender), Dr. Ingo Koch, Dr. Markus Rall, Paul Steidle   
Sitz der Gesellschaft: Offenbach am Main, Registergericht: Amtsgericht Offenbach HRB-Nr. 42592
USt-Ident-Nr. DE 250200933

[-- Attachment #2: pipe.diff --]
[-- Type: application/octet-stream, Size: 2180 bytes --]

Index: pipe.c
===================================================================
--- pipe.c	(revision 3555)
+++ pipe.c	(revision 3638)
@@ -729,8 +729,8 @@
 	return 0;
 }
 
-static ssize_t xnpipe_read(struct file *file,
-			   char *buf, size_t count, loff_t *ppos)
+static ssize_t xnpipe_readv(struct file *file, const struct iovec *iovec,
+			    unsigned long nr_segs, loff_t *ppos)
 {
 	struct xnpipe_state *state = file->private_data;
 	int sigpending, err = 0;
@@ -740,9 +740,6 @@
 	ssize_t ret;
 	spl_t s;
 
-	if (!access_ok(VERIFY_WRITE, buf, count))
-		return -EFAULT;
-
 	xnlock_get_irqsave(&nklock, s);
 
 	if (!testbits(state->status, XNPIPE_KERN_CONN)) {
@@ -781,16 +778,26 @@
 	 * entirely.
 	 */
 
-	inbytes = 0;
+	ret = 0;
 
-	for (;;) {
+	for (; nr_segs--; ++iovec)
+	{
+	    char *buf = iovec->iov_base;
+	    size_t count = iovec->iov_len;
+	
+	    if (!access_ok(VERIFY_WRITE, buf, count)) {
+		    err = -EFAULT;
+		    break;
+	    }
+
+	    for (inbytes = 0; inbytes < count; ) {
 		nbytes = xnpipe_m_size(mh) - xnpipe_m_rdoff(mh);
 
 		if (nbytes + inbytes > count)
 			nbytes = count - inbytes;
 
 		if (nbytes == 0)
-			break;
+			goto cleanup;
 
 		xnlock_put_irqrestore(&nklock, s);
 		/* More data could be appended while doing this: */
@@ -802,15 +809,17 @@
 
 		if (err) {
 			err = -EFAULT;
-			break;
+			goto cleanup;
 		}
 
 		inbytes += nbytes;
 		xnpipe_m_rdoff(mh) += nbytes;
+		ret += nbytes;
+	    }
 	}
 
-	state->ionrd -= inbytes;
-	ret = inbytes;
+cleanup:
+	state->ionrd -= ret;
 
 	if (xnpipe_m_size(mh) > xnpipe_m_rdoff(mh))
 		prependq(&state->outq, &mh->link);
@@ -837,6 +846,13 @@
 	return err ? : ret;
 }
 
+static ssize_t xnpipe_read(struct file *file,
+			   char *buf, size_t count, loff_t *ppos)
+{
+	struct iovec iov = { .iov_base = buf, .iov_len = count };
+	return xnpipe_readv(file, &iov, 1, ppos);
+}
+
 static ssize_t xnpipe_write(struct file *file,
 			    const char *buf, size_t count, loff_t *ppos)
 {
@@ -1060,6 +1076,7 @@
 static struct file_operations xnpipe_fops = {
 	.owner = THIS_MODULE,
 	.read = xnpipe_read,
+	.readv = xnpipe_readv,
 	.write = xnpipe_write,
 	.poll = xnpipe_poll,
 	.ioctl = xnpipe_ioctl,