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

[compudj@krystal.dyndns.org (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