[ltt-dev] [UST PATCH] remove duplicate return

[ltt-dev] [UST PATCH] remove duplicate return

[Douglas Santos]

---
 libustcmd/ustcmd.c |    5 -----
 1 files changed, 0 insertions(+), 5 deletions(-)

diff --git a/libustcmd/ustcmd.c b/libustcmd/ustcmd.c
index cf6b9d7..825a649 100644
--- a/libustcmd/ustcmd.c
+++ b/libustcmd/ustcmd.c
@@ -381,11 +381,6 @@ int ustcmd_get_cmsf(struct marker_status **cmsf, const pid_t pid)
 		return -1;
 	}
 
-	if (result != 1) {
-		ERR("error while getting markers list");
-		return -1;
-	}
-
 	tmp_cmsf = (struct marker_status *) malloc(sizeof(struct marker_status) *
 		(ustcmd_count_nl(big_str) + 1));
 	if (tmp_cmsf == NULL) {
-- 
1.7.0.4

[ltt-dev] [UST PATCH] remove duplicate return

[Mathieu Desnoyers]

* Douglas Santos (douglas.santos at polymtl.ca) wrote:
> ---
>  libustcmd/ustcmd.c |    5 -----
>  1 files changed, 0 insertions(+), 5 deletions(-)
> 
> diff --git a/libustcmd/ustcmd.c b/libustcmd/ustcmd.c
> index cf6b9d7..825a649 100644
> --- a/libustcmd/ustcmd.c
> +++ b/libustcmd/ustcmd.c
> @@ -381,11 +381,6 @@ int ustcmd_get_cmsf(struct marker_status **cmsf, const pid_t pid)
>  		return -1;
>  	}
>  
> -	if (result != 1) {
> -		ERR("error while getting markers list");
> -		return -1;
> -	}

Looks good, so

Acked-by Mathieu Desnoyers <mathieu.desnoyers at efficios.com>

but why on earth is ustcomm_send_request() returning:

/*
 * Return value:
 *   0: Success, but no reply because recv() returned 0
 *   1: Success
 *   -1: Error
 *
 * On error, the error message is printed, except on
 * ECONNRESET, which is normal when the application dies.
 */

Typical return values everywhere else in the project, in the Linux
kernel, and in libs are:

0: success
negative: errors.
positive: used for a quantity counter

So for ustcomm_send_request(), I recommend to remap the "return 0" to
"return -ENODATA".  And to remap "return 1" to return 0, and update all
callers to test for if (ret < 0) rather than if (ret != 1).

If you ever need inspiration for error values, please refer to 
/usr/include/asm-generic/errno-base.h and
/usr/include/asm-generic/errno.h

Thanks,

Mathieu


> -
>  	tmp_cmsf = (struct marker_status *) malloc(sizeof(struct marker_status) *
>  		(ustcmd_count_nl(big_str) + 1));
>  	if (tmp_cmsf == NULL) {
> -- 
> 1.7.0.4
> 
> 
> _______________________________________________
> ltt-dev mailing list
> ltt-dev at lists.casi.polymtl.ca
> http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev
> 

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

[ltt-dev] [UST PATCH] remove duplicate return

[Pierre-Marc Fournier]

I disagree with you Mathieu. These retvals are the same as i/o syscalls (read/write/send/recv/...)and therefore should in my opinion remain as is.

pmf

----- Original message -----
> * Douglas Santos (douglas.santos at polymtl.ca) wrote:
> > ---
> > libustcmd/ustcmd.c |? ? ?  5 -----
> > 1 files changed, 0 insertions(+), 5 deletions(-)
> > 
> > diff --git a/libustcmd/ustcmd.c b/libustcmd/ustcmd.c
> > index cf6b9d7..825a649 100644
> > --- a/libustcmd/ustcmd.c
> > +++ b/libustcmd/ustcmd.c
> > @@ -381,11 +381,6 @@ int ustcmd_get_cmsf(struct marker_status **cmsf,
> > const pid_t pid)??? ??? return -1;
> > ??? }
> > 
> > -??? if (result != 1) {
> > -??? ??? ERR("error while getting markers list");
> > -??? ??? return -1;
> > -??? }
> 
> Looks good, so
> 
> Acked-by Mathieu Desnoyers <mathieu.desnoyers at efficios.com>
> 
> but why on earth is ustcomm_send_request() returning:
> 
> /*
>?  * Return value:
>?  *? ?  0: Success, but no reply because recv() returned 0
>?  *? ?  1: Success
>?  *? ?  -1: Error
>?  *
>?  * On error, the error message is printed, except on
>?  * ECONNRESET, which is normal when the application dies.
>?  */
> 
> Typical return values everywhere else in the project, in the Linux
> kernel, and in libs are:
> 
> 0: success
> negative: errors.
> positive: used for a quantity counter
> 
> So for ustcomm_send_request(), I recommend to remap the "return 0" to
> "return -ENODATA".?  And to remap "return 1" to return 0, and update all
> callers to test for if (ret < 0) rather than if (ret != 1).
> 
> If you ever need inspiration for error values, please refer to 
> /usr/include/asm-generic/errno-base.h and
> /usr/include/asm-generic/errno.h
> 
> Thanks,
> 
> Mathieu
> 
> 
> > -
> > ??? tmp_cmsf = (struct marker_status *) malloc(sizeof(struct
> > marker_status) *??? ??? (ustcmd_count_nl(big_str) + 1));
> > ??? if (tmp_cmsf == NULL) {
> > -- 
> > 1.7.0.4
> > 
> > 
> > _______________________________________________
> > ltt-dev mailing list
> > ltt-dev at lists.casi.polymtl.ca
> > http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev
> > 
> 
> -- 
> Mathieu Desnoyers
> Operating System Efficiency R&D Consultant
> EfficiOS Inc.
> http://www.efficios.com
> 
> _______________________________________________
> ltt-dev mailing list
> ltt-dev at lists.casi.polymtl.ca
> http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev

[ltt-dev] [UST PATCH] remove duplicate return

[Mathieu Desnoyers]

* Pierre-Marc Fournier (pierre-marc.fournier at polymtl.ca) wrote:
> I disagree with you Mathieu. These retvals are the same as i/o
> syscalls (read/write/send/recv/...)and therefore should in my opinion
> remain as is.

Well, this function is not technically the same as i/o syscalls at all.
It uses I/O syscalls, but it is not an I/O syscall per se, so the return
value transformation to a more standard pattern (neg err val, 0 ok)
should happen right in this function rather than to let all callers
handle this. I/O syscalls use positive return values to indicate the
number of bytes read/written/etc. Here, this function arbitrarily choose
1 to indicate that "something has been sent" without caring about the
amount of data moved at all.

So as it doesn't need the whole positive range to spell out the amount
of data moved, it doesn't need to do the same special-cases that the I/O
syscalls are doing. It adds a lot of error values management oddness
without adding anything.

So even though I agree with you that this function is close to the I/O
system calls because it calls it, it is very far from the I/O syscalls
semantically (we don't care about the number of bytes written), and even
though we might be tempted to use the same error values as system calls
for them, the fact that we just don't care about the number of bytes
written combined with the fact that standardizing error value across the
code makes it much easier to follow and to write just call for this
change.

Thanks,

Mathieu

> 
> pmf
> 
> ----- Original message -----
> > * Douglas Santos (douglas.santos at polymtl.ca) wrote:
> > > ---
> > > libustcmd/ustcmd.c |? ? ?  5 -----
> > > 1 files changed, 0 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/libustcmd/ustcmd.c b/libustcmd/ustcmd.c
> > > index cf6b9d7..825a649 100644
> > > --- a/libustcmd/ustcmd.c
> > > +++ b/libustcmd/ustcmd.c
> > > @@ -381,11 +381,6 @@ int ustcmd_get_cmsf(struct marker_status **cmsf,
> > > const pid_t pid)??? ??? return -1;
> > > ??? }
> > > 
> > > -??? if (result != 1) {
> > > -??? ??? ERR("error while getting markers list");
> > > -??? ??? return -1;
> > > -??? }
> > 
> > Looks good, so
> > 
> > Acked-by Mathieu Desnoyers <mathieu.desnoyers at efficios.com>
> > 
> > but why on earth is ustcomm_send_request() returning:
> > 
> > /*
> >?  * Return value:
> >?  *? ?  0: Success, but no reply because recv() returned 0
> >?  *? ?  1: Success
> >?  *? ?  -1: Error
> >?  *
> >?  * On error, the error message is printed, except on
> >?  * ECONNRESET, which is normal when the application dies.
> >?  */
> > 
> > Typical return values everywhere else in the project, in the Linux
> > kernel, and in libs are:
> > 
> > 0: success
> > negative: errors.
> > positive: used for a quantity counter
> > 
> > So for ustcomm_send_request(), I recommend to remap the "return 0" to
> > "return -ENODATA".?  And to remap "return 1" to return 0, and update all
> > callers to test for if (ret < 0) rather than if (ret != 1).
> > 
> > If you ever need inspiration for error values, please refer to 
> > /usr/include/asm-generic/errno-base.h and
> > /usr/include/asm-generic/errno.h
> > 
> > Thanks,
> > 
> > Mathieu
> > 
> > 
> > > -
> > > ??? tmp_cmsf = (struct marker_status *) malloc(sizeof(struct
> > > marker_status) *??? ??? (ustcmd_count_nl(big_str) + 1));
> > > ??? if (tmp_cmsf == NULL) {
> > > -- 
> > > 1.7.0.4
> > > 
> > > 
> > > _______________________________________________
> > > ltt-dev mailing list
> > > ltt-dev at lists.casi.polymtl.ca
> > > http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev
> > > 
> > 
> > -- 
> > Mathieu Desnoyers
> > Operating System Efficiency R&D Consultant
> > EfficiOS Inc.
> > http://www.efficios.com
> > 
> > _______________________________________________
> > ltt-dev mailing list
> > ltt-dev at lists.casi.polymtl.ca
> > http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev
> 
> 
> _______________________________________________
> ltt-dev mailing list
> ltt-dev at lists.casi.polymtl.ca
> http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

[ltt-dev] [UST PATCH] remove duplicate return

[Mathieu Desnoyers]

* Mathieu Desnoyers (compudj at krystal.dyndns.org) wrote:
> * Pierre-Marc Fournier (pierre-marc.fournier at polymtl.ca) wrote:
> > I disagree with you Mathieu. These retvals are the same as i/o
> > syscalls (read/write/send/recv/...)and therefore should in my opinion
> > remain as is.
> 
> Well, this function is not technically the same as i/o syscalls at all.
> It uses I/O syscalls, but it is not an I/O syscall per se, so the return
> value transformation to a more standard pattern (neg err val, 0 ok)
> should happen right in this function rather than to let all callers
> handle this. I/O syscalls use positive return values to indicate the
> number of bytes read/written/etc. Here, this function arbitrarily choose
> 1 to indicate that "something has been sent" without caring about the
> amount of data moved at all.
> 
> So as it doesn't need the whole positive range to spell out the amount
> of data moved, it doesn't need to do the same special-cases that the I/O
> syscalls are doing. It adds a lot of error values management oddness
> without adding anything.
> 
> So even though I agree with you that this function is close to the I/O
> system calls because it calls it, it is very far from the I/O syscalls
> semantically (we don't care about the number of bytes written), and even
> though we might be tempted to use the same error values as system calls
> for them, the fact that we just don't care about the number of bytes
> written combined with the fact that standardizing error value across the
> code makes it much easier to follow and to write just call for this
> change.

By the way, looking at include/share.h:patient_write(), in the case
where write returns 0, I think we should consider this as a success and
loop again to retry write rather than consider that an error occurred.
The same apply to patient_send(). See the manpages for details:

write(2):

RETURN VALUE
       On  success,  the  number  of bytes written is returned (zero indicates
       nothing was written).  On error, -1  is  returned,  and  errno  is  set
       appropriately.

       If  count  is  zero  and  fd refers to a regular file, then write() may
       return a failure status if one of the errors below is detected.  If  no
       errors  are  detected,  0  will  be  returned without causing any other
       effect.  If count is zero and fd refers to a file other than a  regular
       file, the results are not specified.

send(2):
RETURN VALUE
       On success, these calls return  the  number  of  characters sent.   On
       error, -1 is returned, and errno is set appropriately.

Thanks,

Mathieu




> 
> Thanks,
> 
> Mathieu
> 
> > 
> > pmf
> > 
> > ----- Original message -----
> > > * Douglas Santos (douglas.santos at polymtl.ca) wrote:
> > > > ---
> > > > libustcmd/ustcmd.c |? ? ?  5 -----
> > > > 1 files changed, 0 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/libustcmd/ustcmd.c b/libustcmd/ustcmd.c
> > > > index cf6b9d7..825a649 100644
> > > > --- a/libustcmd/ustcmd.c
> > > > +++ b/libustcmd/ustcmd.c
> > > > @@ -381,11 +381,6 @@ int ustcmd_get_cmsf(struct marker_status **cmsf,
> > > > const pid_t pid)??? ??? return -1;
> > > > ??? }
> > > > 
> > > > -??? if (result != 1) {
> > > > -??? ??? ERR("error while getting markers list");
> > > > -??? ??? return -1;
> > > > -??? }
> > > 
> > > Looks good, so
> > > 
> > > Acked-by Mathieu Desnoyers <mathieu.desnoyers at efficios.com>
> > > 
> > > but why on earth is ustcomm_send_request() returning:
> > > 
> > > /*
> > >?  * Return value:
> > >?  *? ?  0: Success, but no reply because recv() returned 0
> > >?  *? ?  1: Success
> > >?  *? ?  -1: Error
> > >?  *
> > >?  * On error, the error message is printed, except on
> > >?  * ECONNRESET, which is normal when the application dies.
> > >?  */
> > > 
> > > Typical return values everywhere else in the project, in the Linux
> > > kernel, and in libs are:
> > > 
> > > 0: success
> > > negative: errors.
> > > positive: used for a quantity counter
> > > 
> > > So for ustcomm_send_request(), I recommend to remap the "return 0" to
> > > "return -ENODATA".?  And to remap "return 1" to return 0, and update all
> > > callers to test for if (ret < 0) rather than if (ret != 1).
> > > 
> > > If you ever need inspiration for error values, please refer to 
> > > /usr/include/asm-generic/errno-base.h and
> > > /usr/include/asm-generic/errno.h
> > > 
> > > Thanks,
> > > 
> > > Mathieu
> > > 
> > > 
> > > > -
> > > > ??? tmp_cmsf = (struct marker_status *) malloc(sizeof(struct
> > > > marker_status) *??? ??? (ustcmd_count_nl(big_str) + 1));
> > > > ??? if (tmp_cmsf == NULL) {
> > > > -- 
> > > > 1.7.0.4
> > > > 
> > > > 
> > > > _______________________________________________
> > > > ltt-dev mailing list
> > > > ltt-dev at lists.casi.polymtl.ca
> > > > http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev
> > > > 
> > > 
> > > -- 
> > > Mathieu Desnoyers
> > > Operating System Efficiency R&D Consultant
> > > EfficiOS Inc.
> > > http://www.efficios.com
> > > 
> > > _______________________________________________
> > > ltt-dev mailing list
> > > ltt-dev at lists.casi.polymtl.ca
> > > http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev
> > 
> > 
> > _______________________________________________
> > ltt-dev mailing list
> > ltt-dev at lists.casi.polymtl.ca
> > http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev
> 
> -- 
> Mathieu Desnoyers
> Operating System Efficiency R&D Consultant
> EfficiOS Inc.
> http://www.efficios.com
> 
> _______________________________________________
> ltt-dev mailing list
> ltt-dev at lists.casi.polymtl.ca
> http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev
> 

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

[ltt-dev] [UST PATCH] remove duplicate return

[Pierre-Marc Fournier]

On 09/06/2010 11:29 AM, Mathieu Desnoyers wrote:
> * Mathieu Desnoyers (compudj at krystal.dyndns.org) wrote:
>> * Pierre-Marc Fournier (pierre-marc.fournier at polymtl.ca) wrote:
>>> I disagree with you Mathieu. These retvals are the same as i/o
>>> syscalls (read/write/send/recv/...)and therefore should in my opinion
>>> remain as is.
>>
>> Well, this function is not technically the same as i/o syscalls at all.
>> It uses I/O syscalls, but it is not an I/O syscall per se, so the return
>> value transformation to a more standard pattern (neg err val, 0 ok)
>> should happen right in this function rather than to let all callers
>> handle this. I/O syscalls use positive return values to indicate the
>> number of bytes read/written/etc. Here, this function arbitrarily choose
>> 1 to indicate that "something has been sent" without caring about the
>> amount of data moved at all.
>>
>> So as it doesn't need the whole positive range to spell out the amount
>> of data moved, it doesn't need to do the same special-cases that the I/O
>> syscalls are doing. It adds a lot of error values management oddness
>> without adding anything.
>>
>> So even though I agree with you that this function is close to the I/O
>> system calls because it calls it, it is very far from the I/O syscalls
>> semantically (we don't care about the number of bytes written), and even
>> though we might be tempted to use the same error values as system calls
>> for them, the fact that we just don't care about the number of bytes
>> written combined with the fact that standardizing error value across the
>> code makes it much easier to follow and to write just call for this
>> change.
>
> By the way, looking at include/share.h:patient_write(), in the case
> where write returns 0, I think we should consider this as a success and
> loop again to retry write rather than consider that an error occurred.
> The same apply to patient_send(). See the manpages for details:
>
> write(2):
>
> RETURN VALUE
>         On  success,  the  number  of bytes written is returned (zero indicates
>         nothing was written).  On error, -1  is  returned,  and  errno  is  set
>         appropriately.
>
>         If  count  is  zero  and  fd refers to a regular file, then write() may
>         return a failure status if one of the errors below is detected.  If  no
>         errors  are  detected,  0  will  be  returned without causing any other
>         effect.  If count is zero and fd refers to a file other than a  regular
>         file, the results are not specified.
>
> send(2):
> RETURN VALUE
>         On success, these calls return  the  number  of  characters sent.   On
>         error, -1 is returned, and errno is set appropriately.
>

No. 0 indicates end of stream and looping on it will result in an 
infinite loop. You need to refer to the specific backend driver to 
understand these specific semantics. The read/write manpages are 
notorious for their non-clarity about this.

pmf

[ltt-dev] [UST PATCH] remove duplicate return

[Mathieu Desnoyers]

* Pierre-Marc Fournier (pierre-marc.fournier at polymtl.ca) wrote:
> On 09/06/2010 11:29 AM, Mathieu Desnoyers wrote:
>> * Mathieu Desnoyers (compudj at krystal.dyndns.org) wrote:
>>> * Pierre-Marc Fournier (pierre-marc.fournier at polymtl.ca) wrote:
>>>> I disagree with you Mathieu. These retvals are the same as i/o
>>>> syscalls (read/write/send/recv/...)and therefore should in my opinion
>>>> remain as is.
>>>
>>> Well, this function is not technically the same as i/o syscalls at all.
>>> It uses I/O syscalls, but it is not an I/O syscall per se, so the return
>>> value transformation to a more standard pattern (neg err val, 0 ok)
>>> should happen right in this function rather than to let all callers
>>> handle this. I/O syscalls use positive return values to indicate the
>>> number of bytes read/written/etc. Here, this function arbitrarily choose
>>> 1 to indicate that "something has been sent" without caring about the
>>> amount of data moved at all.
>>>
>>> So as it doesn't need the whole positive range to spell out the amount
>>> of data moved, it doesn't need to do the same special-cases that the I/O
>>> syscalls are doing. It adds a lot of error values management oddness
>>> without adding anything.
>>>
>>> So even though I agree with you that this function is close to the I/O
>>> system calls because it calls it, it is very far from the I/O syscalls
>>> semantically (we don't care about the number of bytes written), and even
>>> though we might be tempted to use the same error values as system calls
>>> for them, the fact that we just don't care about the number of bytes
>>> written combined with the fact that standardizing error value across the
>>> code makes it much easier to follow and to write just call for this
>>> change.
>>
>> By the way, looking at include/share.h:patient_write(), in the case
>> where write returns 0, I think we should consider this as a success and
>> loop again to retry write rather than consider that an error occurred.
>> The same apply to patient_send(). See the manpages for details:
>>
>> write(2):
>>
>> RETURN VALUE
>>         On  success,  the  number  of bytes written is returned (zero indicates
>>         nothing was written).  On error, -1  is  returned,  and  errno  is  set
>>         appropriately.
>>
>>         If  count  is  zero  and  fd refers to a regular file, then write() may
>>         return a failure status if one of the errors below is detected.  If  no
>>         errors  are  detected,  0  will  be  returned without causing any other
>>         effect.  If count is zero and fd refers to a file other than a  regular
>>         file, the results are not specified.
>>
>> send(2):
>> RETURN VALUE
>>         On success, these calls return  the  number  of  characters sent.   On
>>         error, -1 is returned, and errno is set appropriately.
>>
>
> No. 0 indicates end of stream

By end of stream, you mean this sequence ?

server:
  (block SIGPIPE)
  sockwrite = socket(AF_UNIX, SOCK_STREAM, 0);
  addr.sun_family = AF_UNIX;
  strcpy(addr.sun_path, "./mysocket");

  /* wait for client to create the unix socket */
  sleep(2);

  connect(sockwrite, (struct sockaddr *)&addr, sizeof(addr));)

  /* let's do one write which will be consumed by the client */
  write(sockwrite, "blah", sizeof("blah"));

  /* wait for client to close FD */
  sleep(2);

  ret = write(sockwrite, "blah", sizeof("blah"));
  ( or even with ret = send(sockwrite, "blah", sizeof("blah"), MSG_NOSIGNAL); )
  printf("write ret %ld errno %ld\n", ret, errno);


client:
  (block SIGPIPE)
  sockserv = socket(AF_UNIX, SOCK_STREAM, 0);
  addr.sun_family = AF_UNIX;
  strcpy(addr.sun_path, "./mysocket");
  bind(sockserv, (struct sockaddr *)&addr, sizeof(addr));
  listen(sockserv, 1);
  sockread = accept(sockserv, NULL, NULL);

  recv(sockread, buf, sizeof(buf), 0);

  close(sockread);

The server printf returns:

 "write ret -1 errno 32"

So the return value is -1, with an error number: EPIPE: Broken pipe

> and looping on it will result in an  
> infinite loop. You need to refer to the specific backend driver to  
> understand these specific semantics. The read/write manpages are  
> notorious for their non-clarity about this.

This is what I just did by creating my small test program. Unless I am
misunderstanding your definition of "end of stream", your assumptions
about the way the unix sockets work seem incorrect.

Note that as the manpage says, if the file descriptor refers to a
regular file, then write() can return 0, with an error number set, which
could indicate that an error occured. If the file descriptor refers to a
file other than a regular file, the results are unspecified. So I think
the sane way to handle this would be to check if errno is 0 when write
returns 0. If errno != 0, then we have an error, but if errno == 0, I'd
consider that 0 bytes were written and retry.


By the way, you forgot to reply to my first message above which covers
the topic of semantics, so I'm re-pasting it here.

(about ustcomm_send_request())

Well, this function is not technically the same as i/o syscalls at all.
It uses I/O syscalls, but it is not an I/O syscall per se, so the return
value transformation to a more standard pattern (neg err val, 0 ok)
should happen right in this function rather than to let all callers
handle this. I/O syscalls use positive return values to indicate the
number of bytes read/written/etc. Here, this function arbitrarily choose
1 to indicate that "something has been sent" without caring about the
amount of data moved at all.

So as it doesn't need the whole positive range to spell out the amount
of data moved, it doesn't need to do the same special-cases that the I/O
syscalls are doing. It adds a lot of error values management oddness
without adding anything.

So even though I agree with you that this function is close to the I/O
system calls because it calls it, it is very far from the I/O syscalls
semantically (we don't care about the number of bytes written), and even
though we might be tempted to use the same error values as system calls
for them, the fact that we just don't care about the number of bytes
written combined with the fact that standardizing error value across the
code makes it much easier to follow and to write just call for this
change.


Thanks,

Mathieu

>
> pmf
>

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

[ltt-dev] [UST PATCH] remove duplicate return

[David Goulet]

Following this thread.

Should these be negative value ? (in include/ust/ustcmd.h)

#define USTCMD_ERR_CONN		1 /* Process connection error */
#define USTCMD_ERR_ARG		2 /* Invalid function argument */
#define USTCMD_ERR_GEN		3 /* General ustcmd error */

David

On 10-09-07 11:43 AM, Mathieu Desnoyers wrote:
> * Pierre-Marc Fournier (pierre-marc.fournier at polymtl.ca) wrote:
>> On 09/06/2010 11:29 AM, Mathieu Desnoyers wrote:
>>> * Mathieu Desnoyers (compudj at krystal.dyndns.org) wrote:
>>>> * Pierre-Marc Fournier (pierre-marc.fournier at polymtl.ca) wrote:
>>>>> I disagree with you Mathieu. These retvals are the same as i/o
>>>>> syscalls (read/write/send/recv/...)and therefore should in my opinion
>>>>> remain as is.
>>>>
>>>> Well, this function is not technically the same as i/o syscalls at all.
>>>> It uses I/O syscalls, but it is not an I/O syscall per se, so the return
>>>> value transformation to a more standard pattern (neg err val, 0 ok)
>>>> should happen right in this function rather than to let all callers
>>>> handle this. I/O syscalls use positive return values to indicate the
>>>> number of bytes read/written/etc. Here, this function arbitrarily choose
>>>> 1 to indicate that "something has been sent" without caring about the
>>>> amount of data moved at all.
>>>>
>>>> So as it doesn't need the whole positive range to spell out the amount
>>>> of data moved, it doesn't need to do the same special-cases that the I/O
>>>> syscalls are doing. It adds a lot of error values management oddness
>>>> without adding anything.
>>>>
>>>> So even though I agree with you that this function is close to the I/O
>>>> system calls because it calls it, it is very far from the I/O syscalls
>>>> semantically (we don't care about the number of bytes written), and even
>>>> though we might be tempted to use the same error values as system calls
>>>> for them, the fact that we just don't care about the number of bytes
>>>> written combined with the fact that standardizing error value across the
>>>> code makes it much easier to follow and to write just call for this
>>>> change.
>>>
>>> By the way, looking at include/share.h:patient_write(), in the case
>>> where write returns 0, I think we should consider this as a success and
>>> loop again to retry write rather than consider that an error occurred.
>>> The same apply to patient_send(). See the manpages for details:
>>>
>>> write(2):
>>>
>>> RETURN VALUE
>>>          On  success,  the  number  of bytes written is returned (zero indicates
>>>          nothing was written).  On error, -1  is  returned,  and  errno  is  set
>>>          appropriately.
>>>
>>>          If  count  is  zero  and  fd refers to a regular file, then write() may
>>>          return a failure status if one of the errors below is detected.  If  no
>>>          errors  are  detected,  0  will  be  returned without causing any other
>>>          effect.  If count is zero and fd refers to a file other than a  regular
>>>          file, the results are not specified.
>>>
>>> send(2):
>>> RETURN VALUE
>>>          On success, these calls return  the  number  of  characters sent.   On
>>>          error, -1 is returned, and errno is set appropriately.
>>>
>>
>> No. 0 indicates end of stream
>
> By end of stream, you mean this sequence ?
>
> server:
>    (block SIGPIPE)
>    sockwrite = socket(AF_UNIX, SOCK_STREAM, 0);
>    addr.sun_family = AF_UNIX;
>    strcpy(addr.sun_path, "./mysocket");
>
>    /* wait for client to create the unix socket */
>    sleep(2);
>
>    connect(sockwrite, (struct sockaddr *)&addr, sizeof(addr));)
>
>    /* let's do one write which will be consumed by the client */
>    write(sockwrite, "blah", sizeof("blah"));
>
>    /* wait for client to close FD */
>    sleep(2);
>
>    ret = write(sockwrite, "blah", sizeof("blah"));
>    ( or even with ret = send(sockwrite, "blah", sizeof("blah"), MSG_NOSIGNAL); )
>    printf("write ret %ld errno %ld\n", ret, errno);
>
>
> client:
>    (block SIGPIPE)
>    sockserv = socket(AF_UNIX, SOCK_STREAM, 0);
>    addr.sun_family = AF_UNIX;
>    strcpy(addr.sun_path, "./mysocket");
>    bind(sockserv, (struct sockaddr *)&addr, sizeof(addr));
>    listen(sockserv, 1);
>    sockread = accept(sockserv, NULL, NULL);
>
>    recv(sockread, buf, sizeof(buf), 0);
>
>    close(sockread);
>
> The server printf returns:
>
>   "write ret -1 errno 32"
>
> So the return value is -1, with an error number: EPIPE: Broken pipe
>
>> and looping on it will result in an
>> infinite loop. You need to refer to the specific backend driver to
>> understand these specific semantics. The read/write manpages are
>> notorious for their non-clarity about this.
>
> This is what I just did by creating my small test program. Unless I am
> misunderstanding your definition of "end of stream", your assumptions
> about the way the unix sockets work seem incorrect.
>
> Note that as the manpage says, if the file descriptor refers to a
> regular file, then write() can return 0, with an error number set, which
> could indicate that an error occured. If the file descriptor refers to a
> file other than a regular file, the results are unspecified. So I think
> the sane way to handle this would be to check if errno is 0 when write
> returns 0. If errno != 0, then we have an error, but if errno == 0, I'd
> consider that 0 bytes were written and retry.
>
>
> By the way, you forgot to reply to my first message above which covers
> the topic of semantics, so I'm re-pasting it here.
>
> (about ustcomm_send_request())
>
> Well, this function is not technically the same as i/o syscalls at all.
> It uses I/O syscalls, but it is not an I/O syscall per se, so the return
> value transformation to a more standard pattern (neg err val, 0 ok)
> should happen right in this function rather than to let all callers
> handle this. I/O syscalls use positive return values to indicate the
> number of bytes read/written/etc. Here, this function arbitrarily choose
> 1 to indicate that "something has been sent" without caring about the
> amount of data moved at all.
>
> So as it doesn't need the whole positive range to spell out the amount
> of data moved, it doesn't need to do the same special-cases that the I/O
> syscalls are doing. It adds a lot of error values management oddness
> without adding anything.
>
> So even though I agree with you that this function is close to the I/O
> system calls because it calls it, it is very far from the I/O syscalls
> semantically (we don't care about the number of bytes written), and even
> though we might be tempted to use the same error values as system calls
> for them, the fact that we just don't care about the number of bytes
> written combined with the fact that standardizing error value across the
> code makes it much easier to follow and to write just call for this
> change.
>
>
> Thanks,
>
> Mathieu
>
>>
>> pmf
>>
>

-- 
David Goulet
LTTng project, DORSAL Lab.

PGP/GPG : 1024D/16BD8563
BE3C 672B 9331 9796 291A  14C6 4AF7 C14B 16BD 8563

[ltt-dev] [UST PATCH] remove duplicate return

[Mathieu Desnoyers]

* David Goulet (david.goulet at polymtl.ca) wrote:
> Following this thread.
>
> Should these be negative value ? (in include/ust/ustcmd.h)
>
> #define USTCMD_ERR_CONN		1 /* Process connection error */
> #define USTCMD_ERR_ARG		2 /* Invalid function argument */
> #define USTCMD_ERR_GEN		3 /* General ustcmd error */

Yes. And the caller of ustcmd_set_marker_state should deal with the
return value.

Thanks,

Mathieu

>
> David
>
> On 10-09-07 11:43 AM, Mathieu Desnoyers wrote:
>> * Pierre-Marc Fournier (pierre-marc.fournier at polymtl.ca) wrote:
>>> On 09/06/2010 11:29 AM, Mathieu Desnoyers wrote:
>>>> * Mathieu Desnoyers (compudj at krystal.dyndns.org) wrote:
>>>>> * Pierre-Marc Fournier (pierre-marc.fournier at polymtl.ca) wrote:
>>>>>> I disagree with you Mathieu. These retvals are the same as i/o
>>>>>> syscalls (read/write/send/recv/...)and therefore should in my opinion
>>>>>> remain as is.
>>>>>
>>>>> Well, this function is not technically the same as i/o syscalls at all.
>>>>> It uses I/O syscalls, but it is not an I/O syscall per se, so the return
>>>>> value transformation to a more standard pattern (neg err val, 0 ok)
>>>>> should happen right in this function rather than to let all callers
>>>>> handle this. I/O syscalls use positive return values to indicate the
>>>>> number of bytes read/written/etc. Here, this function arbitrarily choose
>>>>> 1 to indicate that "something has been sent" without caring about the
>>>>> amount of data moved at all.
>>>>>
>>>>> So as it doesn't need the whole positive range to spell out the amount
>>>>> of data moved, it doesn't need to do the same special-cases that the I/O
>>>>> syscalls are doing. It adds a lot of error values management oddness
>>>>> without adding anything.
>>>>>
>>>>> So even though I agree with you that this function is close to the I/O
>>>>> system calls because it calls it, it is very far from the I/O syscalls
>>>>> semantically (we don't care about the number of bytes written), and even
>>>>> though we might be tempted to use the same error values as system calls
>>>>> for them, the fact that we just don't care about the number of bytes
>>>>> written combined with the fact that standardizing error value across the
>>>>> code makes it much easier to follow and to write just call for this
>>>>> change.
>>>>
>>>> By the way, looking at include/share.h:patient_write(), in the case
>>>> where write returns 0, I think we should consider this as a success and
>>>> loop again to retry write rather than consider that an error occurred.
>>>> The same apply to patient_send(). See the manpages for details:
>>>>
>>>> write(2):
>>>>
>>>> RETURN VALUE
>>>>          On  success,  the  number  of bytes written is returned (zero indicates
>>>>          nothing was written).  On error, -1  is  returned,  and  errno  is  set
>>>>          appropriately.
>>>>
>>>>          If  count  is  zero  and  fd refers to a regular file, then write() may
>>>>          return a failure status if one of the errors below is detected.  If  no
>>>>          errors  are  detected,  0  will  be  returned without causing any other
>>>>          effect.  If count is zero and fd refers to a file other than a  regular
>>>>          file, the results are not specified.
>>>>
>>>> send(2):
>>>> RETURN VALUE
>>>>          On success, these calls return  the  number  of  characters sent.   On
>>>>          error, -1 is returned, and errno is set appropriately.
>>>>
>>>
>>> No. 0 indicates end of stream
>>
>> By end of stream, you mean this sequence ?
>>
>> server:
>>    (block SIGPIPE)
>>    sockwrite = socket(AF_UNIX, SOCK_STREAM, 0);
>>    addr.sun_family = AF_UNIX;
>>    strcpy(addr.sun_path, "./mysocket");
>>
>>    /* wait for client to create the unix socket */
>>    sleep(2);
>>
>>    connect(sockwrite, (struct sockaddr *)&addr, sizeof(addr));)
>>
>>    /* let's do one write which will be consumed by the client */
>>    write(sockwrite, "blah", sizeof("blah"));
>>
>>    /* wait for client to close FD */
>>    sleep(2);
>>
>>    ret = write(sockwrite, "blah", sizeof("blah"));
>>    ( or even with ret = send(sockwrite, "blah", sizeof("blah"), MSG_NOSIGNAL); )
>>    printf("write ret %ld errno %ld\n", ret, errno);
>>
>>
>> client:
>>    (block SIGPIPE)
>>    sockserv = socket(AF_UNIX, SOCK_STREAM, 0);
>>    addr.sun_family = AF_UNIX;
>>    strcpy(addr.sun_path, "./mysocket");
>>    bind(sockserv, (struct sockaddr *)&addr, sizeof(addr));
>>    listen(sockserv, 1);
>>    sockread = accept(sockserv, NULL, NULL);
>>
>>    recv(sockread, buf, sizeof(buf), 0);
>>
>>    close(sockread);
>>
>> The server printf returns:
>>
>>   "write ret -1 errno 32"
>>
>> So the return value is -1, with an error number: EPIPE: Broken pipe
>>
>>> and looping on it will result in an
>>> infinite loop. You need to refer to the specific backend driver to
>>> understand these specific semantics. The read/write manpages are
>>> notorious for their non-clarity about this.
>>
>> This is what I just did by creating my small test program. Unless I am
>> misunderstanding your definition of "end of stream", your assumptions
>> about the way the unix sockets work seem incorrect.
>>
>> Note that as the manpage says, if the file descriptor refers to a
>> regular file, then write() can return 0, with an error number set, which
>> could indicate that an error occured. If the file descriptor refers to a
>> file other than a regular file, the results are unspecified. So I think
>> the sane way to handle this would be to check if errno is 0 when write
>> returns 0. If errno != 0, then we have an error, but if errno == 0, I'd
>> consider that 0 bytes were written and retry.
>>
>>
>> By the way, you forgot to reply to my first message above which covers
>> the topic of semantics, so I'm re-pasting it here.
>>
>> (about ustcomm_send_request())
>>
>> Well, this function is not technically the same as i/o syscalls at all.
>> It uses I/O syscalls, but it is not an I/O syscall per se, so the return
>> value transformation to a more standard pattern (neg err val, 0 ok)
>> should happen right in this function rather than to let all callers
>> handle this. I/O syscalls use positive return values to indicate the
>> number of bytes read/written/etc. Here, this function arbitrarily choose
>> 1 to indicate that "something has been sent" without caring about the
>> amount of data moved at all.
>>
>> So as it doesn't need the whole positive range to spell out the amount
>> of data moved, it doesn't need to do the same special-cases that the I/O
>> syscalls are doing. It adds a lot of error values management oddness
>> without adding anything.
>>
>> So even though I agree with you that this function is close to the I/O
>> system calls because it calls it, it is very far from the I/O syscalls
>> semantically (we don't care about the number of bytes written), and even
>> though we might be tempted to use the same error values as system calls
>> for them, the fact that we just don't care about the number of bytes
>> written combined with the fact that standardizing error value across the
>> code makes it much easier to follow and to write just call for this
>> change.
>>
>>
>> Thanks,
>>
>> Mathieu
>>
>>>
>>> pmf
>>>
>>
>
> -- 
> David Goulet
> LTTng project, DORSAL Lab.
>
> PGP/GPG : 1024D/16BD8563
> BE3C 672B 9331 9796 291A  14C6 4AF7 C14B 16BD 8563
>

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

[ltt-dev] [UST PATCH] remove duplicate return

[Pierre-Marc Fournier]

On 09/07/2010 11:43 AM, Mathieu Desnoyers wrote:
> * Pierre-Marc Fournier (pierre-marc.fournier at polymtl.ca) wrote:
>> On 09/06/2010 11:29 AM, Mathieu Desnoyers wrote:
>>> * Mathieu Desnoyers (compudj at krystal.dyndns.org) wrote:
>>>> * Pierre-Marc Fournier (pierre-marc.fournier at polymtl.ca) wrote:
>>>>> I disagree with you Mathieu. These retvals are the same as i/o
>>>>> syscalls (read/write/send/recv/...)and therefore should in my opinion
>>>>> remain as is.
>>>>
>>>> Well, this function is not technically the same as i/o syscalls at all.
>>>> It uses I/O syscalls, but it is not an I/O syscall per se, so the return
>>>> value transformation to a more standard pattern (neg err val, 0 ok)
>>>> should happen right in this function rather than to let all callers
>>>> handle this. I/O syscalls use positive return values to indicate the
>>>> number of bytes read/written/etc. Here, this function arbitrarily choose
>>>> 1 to indicate that "something has been sent" without caring about the
>>>> amount of data moved at all.
>>>>
>>>> So as it doesn't need the whole positive range to spell out the amount
>>>> of data moved, it doesn't need to do the same special-cases that the I/O
>>>> syscalls are doing. It adds a lot of error values management oddness
>>>> without adding anything.
>>>>
>>>> So even though I agree with you that this function is close to the I/O
>>>> system calls because it calls it, it is very far from the I/O syscalls
>>>> semantically (we don't care about the number of bytes written), and even
>>>> though we might be tempted to use the same error values as system calls
>>>> for them, the fact that we just don't care about the number of bytes
>>>> written combined with the fact that standardizing error value across the
>>>> code makes it much easier to follow and to write just call for this
>>>> change.
>>>
>>> By the way, looking at include/share.h:patient_write(), in the case
>>> where write returns 0, I think we should consider this as a success and
>>> loop again to retry write rather than consider that an error occurred.
>>> The same apply to patient_send(). See the manpages for details:
>>>
>>> write(2):
>>>
>>> RETURN VALUE
>>>          On  success,  the  number  of bytes written is returned (zero indicates
>>>          nothing was written).  On error, -1  is  returned,  and  errno  is  set
>>>          appropriately.
>>>
>>>          If  count  is  zero  and  fd refers to a regular file, then write() may
>>>          return a failure status if one of the errors below is detected.  If  no
>>>          errors  are  detected,  0  will  be  returned without causing any other
>>>          effect.  If count is zero and fd refers to a file other than a  regular
>>>          file, the results are not specified.
>>>
>>> send(2):
>>> RETURN VALUE
>>>          On success, these calls return  the  number  of  characters sent.   On
>>>          error, -1 is returned, and errno is set appropriately.
>>>
>>
>> No. 0 indicates end of stream
>
> By end of stream, you mean this sequence ?
>
> server:
>    (block SIGPIPE)
>    sockwrite = socket(AF_UNIX, SOCK_STREAM, 0);
>    addr.sun_family = AF_UNIX;
>    strcpy(addr.sun_path, "./mysocket");
>
>    /* wait for client to create the unix socket */
>    sleep(2);
>
>    connect(sockwrite, (struct sockaddr *)&addr, sizeof(addr));)
>
>    /* let's do one write which will be consumed by the client */
>    write(sockwrite, "blah", sizeof("blah"));
>
>    /* wait for client to close FD */
>    sleep(2);
>
>    ret = write(sockwrite, "blah", sizeof("blah"));
>    ( or even with ret = send(sockwrite, "blah", sizeof("blah"), MSG_NOSIGNAL); )
>    printf("write ret %ld errno %ld\n", ret, errno);
>
>
> client:
>    (block SIGPIPE)
>    sockserv = socket(AF_UNIX, SOCK_STREAM, 0);
>    addr.sun_family = AF_UNIX;
>    strcpy(addr.sun_path, "./mysocket");
>    bind(sockserv, (struct sockaddr *)&addr, sizeof(addr));
>    listen(sockserv, 1);
>    sockread = accept(sockserv, NULL, NULL);
>
>    recv(sockread, buf, sizeof(buf), 0);
>
>    close(sockread);
>
> The server printf returns:
>
>   "write ret -1 errno 32"
>
> So the return value is -1, with an error number: EPIPE: Broken pipe
>
>> and looping on it will result in an
>> infinite loop. You need to refer to the specific backend driver to
>> understand these specific semantics. The read/write manpages are
>> notorious for their non-clarity about this.
>
> This is what I just did by creating my small test program. Unless I am
> misunderstanding your definition of "end of stream", your assumptions
> about the way the unix sockets work seem incorrect.

Well, just that one assumption. But I accept your explanation and my 
foolishness. ;-) patient_write() and patient_send() need to be fixed as 
you describe.

>
> Note that as the manpage says, if the file descriptor refers to a
> regular file, then write() can return 0, with an error number set, which
> could indicate that an error occured. If the file descriptor refers to a
> file other than a regular file, the results are unspecified. So I think
> the sane way to handle this would be to check if errno is 0 when write
> returns 0. If errno != 0, then we have an error, but if errno == 0, I'd
> consider that 0 bytes were written and retry.
>
>
> By the way, you forgot to reply to my first message above which covers
> the topic of semantics, so I'm re-pasting it here.
>
> (about ustcomm_send_request())
>
> Well, this function is not technically the same as i/o syscalls at all.
> It uses I/O syscalls, but it is not an I/O syscall per se, so the return
> value transformation to a more standard pattern (neg err val, 0 ok)
> should happen right in this function rather than to let all callers
> handle this. I/O syscalls use positive return values to indicate the
> number of bytes read/written/etc. Here, this function arbitrarily choose
> 1 to indicate that "something has been sent" without caring about the
> amount of data moved at all.

Let me copy the description of the return values here.

/*
  * Return value:
  *   0: Success, but no reply because recv() returned 0
  *   1: Success
  *   -1: Error
  *
  * On error, the error message is printed, except on
  * ECONNRESET, which is normal when the application dies.
  */

>
> So as it doesn't need the whole positive range to spell out the amount
> of data moved, it doesn't need to do the same special-cases that the I/O
> syscalls are doing. It adds a lot of error values management oddness
> without adding anything.

There are 3 cases the caller must consider when calling ustcomm_send_msg().
- An error occurred, there is a problem and it is necessary to complain (-1)
- The remote connection was closed so we were not able to send the 
message (0)
- Everything worked fine (1)

Callers need to know if the connection was closed in order not to 
complain but at the same time take consequent measures.

>
> So even though I agree with you that this function is close to the I/O
> system calls because it calls it, it is very far from the I/O syscalls
> semantically (we don't care about the number of bytes written), and even
> though we might be tempted to use the same error values as system calls
> for them, the fact that we just don't care about the number of bytes
> written combined with the fact that standardizing error value across the
> code makes it much easier to follow and to write just call for this
> change.

If you want to propose different semantics that still allow the caller 
to know which of the 3 cases happened, I'm all open.

Thanks

pmf

[ltt-dev] [UST PATCH] remove duplicate return

[Mathieu Desnoyers]

* Pierre-Marc Fournier (pierre-marc.fournier at polymtl.ca) wrote:
> On 09/07/2010 11:43 AM, Mathieu Desnoyers wrote:
>> * Pierre-Marc Fournier (pierre-marc.fournier at polymtl.ca) wrote:
>>> On 09/06/2010 11:29 AM, Mathieu Desnoyers wrote:
>>>> * Mathieu Desnoyers (compudj at krystal.dyndns.org) wrote:
>>>>> * Pierre-Marc Fournier (pierre-marc.fournier at polymtl.ca) wrote:
>>>>>> I disagree with you Mathieu. These retvals are the same as i/o
>>>>>> syscalls (read/write/send/recv/...)and therefore should in my opinion
>>>>>> remain as is.
>>>>>
>>>>> Well, this function is not technically the same as i/o syscalls at all.
>>>>> It uses I/O syscalls, but it is not an I/O syscall per se, so the return
>>>>> value transformation to a more standard pattern (neg err val, 0 ok)
>>>>> should happen right in this function rather than to let all callers
>>>>> handle this. I/O syscalls use positive return values to indicate the
>>>>> number of bytes read/written/etc. Here, this function arbitrarily choose
>>>>> 1 to indicate that "something has been sent" without caring about the
>>>>> amount of data moved at all.
>>>>>
>>>>> So as it doesn't need the whole positive range to spell out the amount
>>>>> of data moved, it doesn't need to do the same special-cases that the I/O
>>>>> syscalls are doing. It adds a lot of error values management oddness
>>>>> without adding anything.
>>>>>
>>>>> So even though I agree with you that this function is close to the I/O
>>>>> system calls because it calls it, it is very far from the I/O syscalls
>>>>> semantically (we don't care about the number of bytes written), and even
>>>>> though we might be tempted to use the same error values as system calls
>>>>> for them, the fact that we just don't care about the number of bytes
>>>>> written combined with the fact that standardizing error value across the
>>>>> code makes it much easier to follow and to write just call for this
>>>>> change.
>>>>
>>>> By the way, looking at include/share.h:patient_write(), in the case
>>>> where write returns 0, I think we should consider this as a success and
>>>> loop again to retry write rather than consider that an error occurred.
>>>> The same apply to patient_send(). See the manpages for details:
>>>>
>>>> write(2):
>>>>
>>>> RETURN VALUE
>>>>          On  success,  the  number  of bytes written is returned (zero indicates
>>>>          nothing was written).  On error, -1  is  returned,  and  errno  is  set
>>>>          appropriately.
>>>>
>>>>          If  count  is  zero  and  fd refers to a regular file, then write() may
>>>>          return a failure status if one of the errors below is detected.  If  no
>>>>          errors  are  detected,  0  will  be  returned without causing any other
>>>>          effect.  If count is zero and fd refers to a file other than a  regular
>>>>          file, the results are not specified.
>>>>
>>>> send(2):
>>>> RETURN VALUE
>>>>          On success, these calls return  the  number  of  characters sent.   On
>>>>          error, -1 is returned, and errno is set appropriately.
>>>>
>>>
>>> No. 0 indicates end of stream
>>
>> By end of stream, you mean this sequence ?
>>
>> server:
>>    (block SIGPIPE)
>>    sockwrite = socket(AF_UNIX, SOCK_STREAM, 0);
>>    addr.sun_family = AF_UNIX;
>>    strcpy(addr.sun_path, "./mysocket");
>>
>>    /* wait for client to create the unix socket */
>>    sleep(2);
>>
>>    connect(sockwrite, (struct sockaddr *)&addr, sizeof(addr));)
>>
>>    /* let's do one write which will be consumed by the client */
>>    write(sockwrite, "blah", sizeof("blah"));
>>
>>    /* wait for client to close FD */
>>    sleep(2);
>>
>>    ret = write(sockwrite, "blah", sizeof("blah"));
>>    ( or even with ret = send(sockwrite, "blah", sizeof("blah"), MSG_NOSIGNAL); )
>>    printf("write ret %ld errno %ld\n", ret, errno);
>>
>>
>> client:
>>    (block SIGPIPE)
>>    sockserv = socket(AF_UNIX, SOCK_STREAM, 0);
>>    addr.sun_family = AF_UNIX;
>>    strcpy(addr.sun_path, "./mysocket");
>>    bind(sockserv, (struct sockaddr *)&addr, sizeof(addr));
>>    listen(sockserv, 1);
>>    sockread = accept(sockserv, NULL, NULL);
>>
>>    recv(sockread, buf, sizeof(buf), 0);
>>
>>    close(sockread);
>>
>> The server printf returns:
>>
>>   "write ret -1 errno 32"
>>
>> So the return value is -1, with an error number: EPIPE: Broken pipe
>>
>>> and looping on it will result in an
>>> infinite loop. You need to refer to the specific backend driver to
>>> understand these specific semantics. The read/write manpages are
>>> notorious for their non-clarity about this.
>>
>> This is what I just did by creating my small test program. Unless I am
>> misunderstanding your definition of "end of stream", your assumptions
>> about the way the unix sockets work seem incorrect.
>
> Well, just that one assumption. But I accept your explanation and my  
> foolishness. ;-) patient_write() and patient_send() need to be fixed as  
> you describe.

OK, is Nils or David willing to take the ball ?

>
>>
>> Note that as the manpage says, if the file descriptor refers to a
>> regular file, then write() can return 0, with an error number set, which
>> could indicate that an error occured. If the file descriptor refers to a
>> file other than a regular file, the results are unspecified. So I think
>> the sane way to handle this would be to check if errno is 0 when write
>> returns 0. If errno != 0, then we have an error, but if errno == 0, I'd
>> consider that 0 bytes were written and retry.
>>
>>
>> By the way, you forgot to reply to my first message above which covers
>> the topic of semantics, so I'm re-pasting it here.
>>
>> (about ustcomm_send_request())
>>
>> Well, this function is not technically the same as i/o syscalls at all.
>> It uses I/O syscalls, but it is not an I/O syscall per se, so the return
>> value transformation to a more standard pattern (neg err val, 0 ok)
>> should happen right in this function rather than to let all callers
>> handle this. I/O syscalls use positive return values to indicate the
>> number of bytes read/written/etc. Here, this function arbitrarily choose
>> 1 to indicate that "something has been sent" without caring about the
>> amount of data moved at all.
>
> Let me copy the description of the return values here.
>
> /*
>  * Return value:
>  *   0: Success, but no reply because recv() returned 0
>  *   1: Success
>  *   -1: Error
>  *
>  * On error, the error message is printed, except on
>  * ECONNRESET, which is normal when the application dies.
>  */
>
>>
>> So as it doesn't need the whole positive range to spell out the amount
>> of data moved, it doesn't need to do the same special-cases that the I/O
>> syscalls are doing. It adds a lot of error values management oddness
>> without adding anything.
>
> There are 3 cases the caller must consider when calling ustcomm_send_msg().
> - An error occurred, there is a problem and it is necessary to complain (-1)
> - The remote connection was closed so we were not able to send the  
> message (0)
> - Everything worked fine (1)
>
> Callers need to know if the connection was closed in order not to  
> complain but at the same time take consequent measures.
>
>>
>> So even though I agree with you that this function is close to the I/O
>> system calls because it calls it, it is very far from the I/O syscalls
>> semantically (we don't care about the number of bytes written), and even
>> though we might be tempted to use the same error values as system calls
>> for them, the fact that we just don't care about the number of bytes
>> written combined with the fact that standardizing error value across the
>> code makes it much easier to follow and to write just call for this
>> change.
>
> If you want to propose different semantics that still allow the caller  
> to know which of the 3 cases happened, I'm all open.

Yep, this is what I am proposing:

return 0: everything is OK
return -EPIPE: remote connection was closed, so we were unable to send
               the message.
return -EWHATEVER (other than -EPIPE): An error occured.

So the caller can deal with -EPIPE as it wants.

How does that sound ?

Thanks,

Mathieu

>
> Thanks
>
> pmf
>

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

[ltt-dev] [UST PATCH] remove duplicate return

[David Goulet]

On 10-09-09 12:38 PM, Mathieu Desnoyers wrote:
> * Pierre-Marc Fournier (pierre-marc.fournier at polymtl.ca) wrote:
>> On 09/07/2010 11:43 AM, Mathieu Desnoyers wrote:
>>> * Pierre-Marc Fournier (pierre-marc.fournier at polymtl.ca) wrote:
>>>> On 09/06/2010 11:29 AM, Mathieu Desnoyers wrote:
>>>>> * Mathieu Desnoyers (compudj at krystal.dyndns.org) wrote:
>>>>>> * Pierre-Marc Fournier (pierre-marc.fournier at polymtl.ca) wrote:
>>>>>>> I disagree with you Mathieu. These retvals are the same as i/o
>>>>>>> syscalls (read/write/send/recv/...)and therefore should in my opinion
>>>>>>> remain as is.
>>>>>>
>>>>>> Well, this function is not technically the same as i/o syscalls at all.
>>>>>> It uses I/O syscalls, but it is not an I/O syscall per se, so the return
>>>>>> value transformation to a more standard pattern (neg err val, 0 ok)
>>>>>> should happen right in this function rather than to let all callers
>>>>>> handle this. I/O syscalls use positive return values to indicate the
>>>>>> number of bytes read/written/etc. Here, this function arbitrarily choose
>>>>>> 1 to indicate that "something has been sent" without caring about the
>>>>>> amount of data moved at all.
>>>>>>
>>>>>> So as it doesn't need the whole positive range to spell out the amount
>>>>>> of data moved, it doesn't need to do the same special-cases that the I/O
>>>>>> syscalls are doing. It adds a lot of error values management oddness
>>>>>> without adding anything.
>>>>>>
>>>>>> So even though I agree with you that this function is close to the I/O
>>>>>> system calls because it calls it, it is very far from the I/O syscalls
>>>>>> semantically (we don't care about the number of bytes written), and even
>>>>>> though we might be tempted to use the same error values as system calls
>>>>>> for them, the fact that we just don't care about the number of bytes
>>>>>> written combined with the fact that standardizing error value across the
>>>>>> code makes it much easier to follow and to write just call for this
>>>>>> change.
>>>>>
>>>>> By the way, looking at include/share.h:patient_write(), in the case
>>>>> where write returns 0, I think we should consider this as a success and
>>>>> loop again to retry write rather than consider that an error occurred.
>>>>> The same apply to patient_send(). See the manpages for details:
>>>>>
>>>>> write(2):
>>>>>
>>>>> RETURN VALUE
>>>>>           On  success,  the  number  of bytes written is returned (zero indicates
>>>>>           nothing was written).  On error, -1  is  returned,  and  errno  is  set
>>>>>           appropriately.
>>>>>
>>>>>           If  count  is  zero  and  fd refers to a regular file, then write() may
>>>>>           return a failure status if one of the errors below is detected.  If  no
>>>>>           errors  are  detected,  0  will  be  returned without causing any other
>>>>>           effect.  If count is zero and fd refers to a file other than a  regular
>>>>>           file, the results are not specified.
>>>>>
>>>>> send(2):
>>>>> RETURN VALUE
>>>>>           On success, these calls return  the  number  of  characters sent.   On
>>>>>           error, -1 is returned, and errno is set appropriately.
>>>>>
>>>>
>>>> No. 0 indicates end of stream
>>>
>>> By end of stream, you mean this sequence ?
>>>
>>> server:
>>>     (block SIGPIPE)
>>>     sockwrite = socket(AF_UNIX, SOCK_STREAM, 0);
>>>     addr.sun_family = AF_UNIX;
>>>     strcpy(addr.sun_path, "./mysocket");
>>>
>>>     /* wait for client to create the unix socket */
>>>     sleep(2);
>>>
>>>     connect(sockwrite, (struct sockaddr *)&addr, sizeof(addr));)
>>>
>>>     /* let's do one write which will be consumed by the client */
>>>     write(sockwrite, "blah", sizeof("blah"));
>>>
>>>     /* wait for client to close FD */
>>>     sleep(2);
>>>
>>>     ret = write(sockwrite, "blah", sizeof("blah"));
>>>     ( or even with ret = send(sockwrite, "blah", sizeof("blah"), MSG_NOSIGNAL); )
>>>     printf("write ret %ld errno %ld\n", ret, errno);
>>>
>>>
>>> client:
>>>     (block SIGPIPE)
>>>     sockserv = socket(AF_UNIX, SOCK_STREAM, 0);
>>>     addr.sun_family = AF_UNIX;
>>>     strcpy(addr.sun_path, "./mysocket");
>>>     bind(sockserv, (struct sockaddr *)&addr, sizeof(addr));
>>>     listen(sockserv, 1);
>>>     sockread = accept(sockserv, NULL, NULL);
>>>
>>>     recv(sockread, buf, sizeof(buf), 0);
>>>
>>>     close(sockread);
>>>
>>> The server printf returns:
>>>
>>>    "write ret -1 errno 32"
>>>
>>> So the return value is -1, with an error number: EPIPE: Broken pipe
>>>
>>>> and looping on it will result in an
>>>> infinite loop. You need to refer to the specific backend driver to
>>>> understand these specific semantics. The read/write manpages are
>>>> notorious for their non-clarity about this.
>>>
>>> This is what I just did by creating my small test program. Unless I am
>>> misunderstanding your definition of "end of stream", your assumptions
>>> about the way the unix sockets work seem incorrect.
>>
>> Well, just that one assumption. But I accept your explanation and my
>> foolishness. ;-) patient_write() and patient_send() need to be fixed as
>> you describe.
>
> OK, is Nils or David willing to take the ball ?
>

It's a time factor on my part. I don't think this is quite urgent so 
I'll try to make it as soon as I can. I know that Nils is quite busy on 
Trace events so I can do that to offload him.

About that, while doing this, I propose we just pass over UST code to 
correct and standardize the error handling and integrate your 
proposition below systematically. I came across a lot of code in ustctl, 
ustcmd and ustcomm that was not quite following the error "convention" 
of UST. Also, as I mention in an early post, the UST "standard" error 
(usterr.h) are positive value so we might also refactor that.

However, this might modify a lot of code so mistakes or missing part can 
easily be forgotten. It will be very important that a careful review be 
done. I'm willing to do that in a near future but, as I said, time 
factor on my part will delay this patch set.

Thanks
David

>>
>>>
>>> Note that as the manpage says, if the file descriptor refers to a
>>> regular file, then write() can return 0, with an error number set, which
>>> could indicate that an error occured. If the file descriptor refers to a
>>> file other than a regular file, the results are unspecified. So I think
>>> the sane way to handle this would be to check if errno is 0 when write
>>> returns 0. If errno != 0, then we have an error, but if errno == 0, I'd
>>> consider that 0 bytes were written and retry.
>>>
>>>
>>> By the way, you forgot to reply to my first message above which covers
>>> the topic of semantics, so I'm re-pasting it here.
>>>
>>> (about ustcomm_send_request())
>>>
>>> Well, this function is not technically the same as i/o syscalls at all.
>>> It uses I/O syscalls, but it is not an I/O syscall per se, so the return
>>> value transformation to a more standard pattern (neg err val, 0 ok)
>>> should happen right in this function rather than to let all callers
>>> handle this. I/O syscalls use positive return values to indicate the
>>> number of bytes read/written/etc. Here, this function arbitrarily choose
>>> 1 to indicate that "something has been sent" without caring about the
>>> amount of data moved at all.
>>
>> Let me copy the description of the return values here.
>>
>> /*
>>   * Return value:
>>   *   0: Success, but no reply because recv() returned 0
>>   *   1: Success
>>   *   -1: Error
>>   *
>>   * On error, the error message is printed, except on
>>   * ECONNRESET, which is normal when the application dies.
>>   */
>>
>>>
>>> So as it doesn't need the whole positive range to spell out the amount
>>> of data moved, it doesn't need to do the same special-cases that the I/O
>>> syscalls are doing. It adds a lot of error values management oddness
>>> without adding anything.
>>
>> There are 3 cases the caller must consider when calling ustcomm_send_msg().
>> - An error occurred, there is a problem and it is necessary to complain (-1)
>> - The remote connection was closed so we were not able to send the
>> message (0)
>> - Everything worked fine (1)
>>
>> Callers need to know if the connection was closed in order not to
>> complain but at the same time take consequent measures.
>>
>>>
>>> So even though I agree with you that this function is close to the I/O
>>> system calls because it calls it, it is very far from the I/O syscalls
>>> semantically (we don't care about the number of bytes written), and even
>>> though we might be tempted to use the same error values as system calls
>>> for them, the fact that we just don't care about the number of bytes
>>> written combined with the fact that standardizing error value across the
>>> code makes it much easier to follow and to write just call for this
>>> change.
>>
>> If you want to propose different semantics that still allow the caller
>> to know which of the 3 cases happened, I'm all open.
>
> Yep, this is what I am proposing:
>
> return 0: everything is OK
> return -EPIPE: remote connection was closed, so we were unable to send
>                 the message.
> return -EWHATEVER (other than -EPIPE): An error occured.
>
> So the caller can deal with -EPIPE as it wants.
>
> How does that sound ?
>
> Thanks,
>
> Mathieu
>
>>
>> Thanks
>>
>> pmf
>>
>

-- 
David Goulet
LTTng project, DORSAL Lab.

PGP/GPG : 1024D/16BD8563
BE3C 672B 9331 9796 291A  14C6 4AF7 C14B 16BD 8563

[ltt-dev] [UST PATCH] remove duplicate return

[Nils Carlson]

Could you strike the other return statement? So we keep the error  
message?

/Nils
On Sep 6, 2010, at 1:26 AM, Douglas Santos wrote:

> ---
> libustcmd/ustcmd.c |    5 -----
> 1 files changed, 0 insertions(+), 5 deletions(-)
>
> diff --git a/libustcmd/ustcmd.c b/libustcmd/ustcmd.c
> index cf6b9d7..825a649 100644
> --- a/libustcmd/ustcmd.c
> +++ b/libustcmd/ustcmd.c
> @@ -381,11 +381,6 @@ int ustcmd_get_cmsf(struct marker_status  
> **cmsf, const pid_t pid)
> 		return -1;
> 	}
>
> -	if (result != 1) {
> -		ERR("error while getting markers list");
> -		return -1;
> -	}
> -
> 	tmp_cmsf = (struct marker_status *) malloc(sizeof(struct  
> marker_status) *
> 		(ustcmd_count_nl(big_str) + 1));
> 	if (tmp_cmsf == NULL) {
> -- 
> 1.7.0.4
>
>
> _______________________________________________
> ltt-dev mailing list
> ltt-dev at lists.casi.polymtl.ca
> http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev