From mboxrd@z Thu Jan 1 00:00:00 1970 From: compudj@krystal.dyndns.org (Mathieu Desnoyers) Date: Mon, 6 Sep 2010 11:29:27 -0400 Subject: [ltt-dev] [UST PATCH] remove duplicate return In-Reply-To: <20100906024254.GA25072@Krystal> References: <1283729203-4435-1-git-send-email-douglas.santos@polymtl.ca> <20100906004907.GB14418@Krystal> <1283735019.1893.3.camel@Nokia-N900-42-11> <20100906024254.GA25072@Krystal> Message-ID: <20100906152927.GA22968@Krystal> * 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 > > > > > > 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