From mboxrd@z Thu Jan 1 00:00:00 1970 From: david.goulet@polymtl.ca (David Goulet) Date: Thu, 09 Sep 2010 13:05:48 -0400 Subject: [ltt-dev] [UST PATCH] remove duplicate return In-Reply-To: <20100909163856.GA15254@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> <20100906152927.GA22968@Krystal> <4C854BA0.4040701@polymtl.ca> <20100907154322.GB6371@Krystal> <4C888863.1080802@polymtl.ca> <20100909163856.GA15254@Krystal> Message-ID: <4C8913EC.7010100@polymtl.ca> 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