Hi!
Yes this is a bug fix so it will go into all branches. Thanks for
comments. Se my replies inline.
regards,
Anders Widell
2013-12-06 08:08, Ramesh Betham skrev:
> Hi Anders,
>
> Ack. Comments inline.
> I hope some of these changes will reflect in later branches of 4.2.x
> i.e., on 4.3.x and default as well.
>
> Thanks,
> Ramesh.
>
> On 12/5/2013 6:42 PM, Anders Widell wrote:
>> osaf/services/infrastructure/nid/nodeinit.c | 9 +++++++--
>> 1 files changed, 7 insertions(+), 2 deletions(-)
>>
>>
>> There were three loops in nodeinit.c that could potentially become
>> infinite if a
>> system call returns -1. It is fine to try again when e.g. errno ==
>> EINTR, since
>> that is a temporary error that will go away eventually. In general
>> however,
>> trying again will not make the error go away and the loop would
>> become infinite.
>> Also made a file descriptor non-blocking to avoid blocking
>> indefinitely in
>> read().
>>
>> diff --git a/osaf/services/infrastructure/nid/nodeinit.c
>> b/osaf/services/infrastructure/nid/nodeinit.c
>> --- a/osaf/services/infrastructure/nid/nodeinit.c
>> +++ b/osaf/services/infrastructure/nid/nodeinit.c
>> @@ -46,6 +46,8 @@
>> * any
>> notification. *
>> ************************************************************************/
>> +#include <unistd.h>
>> +#include <fcntl.h>
>> #include <syslog.h>
>> #include <libgen.h>
>> #include <ctype.h>
>> @@ -621,6 +623,8 @@ int32_t fork_daemon(NID_SPAWN_INFO *serv
>> if(-1 == pipe(filedes))
>> LOG_ER("Problem creating pipe: %s", strerror(errno));
>> + else
>> + fcntl(filedes[0], F_SETFL, fcntl(filedes[0], F_GETFL) |
>> O_NONBLOCK);
>> if ((pid = fork()) == 0) {
>> if (nis_fifofd > 0)
>> @@ -641,10 +645,10 @@ int32_t fork_daemon(NID_SPAWN_INFO *serv
>> continue;
>> else if (errno == EPIPE) {
>> LOG_ER("Reader not available to return my PID");
>> - exit(2);
>> } else {
>> LOG_ER("Problem writing to pipe, err=%s",
>> strerror(errno));
>> }
>> + exit(2);
> [Ramesh]: How about retrying for fixed number of times for EAGAIN errno.
[AndersW] Only the read end of the pipe was made non-blocking above, so
the write() here should not return EAGAIN. Maybe the write-end ought to
be non-blocking too, though. But no matter if it is non-blocking or not
I don't see any possibility that write() would block or return EAGAIN,
since this is the first write to a newly created pipe. There should
always be sufficient space in the pipe buffer (several kilobytes, I think).
>> }
>> setsid();
>> @@ -691,6 +695,7 @@ int32_t fork_daemon(NID_SPAWN_INFO *serv
>> }
>> if (errno == EINTR)
>> continue;
>> + break;
> [Ramesh]: Below of this while-loop there is a while-loop for read(),
> in this case too can we consider for retry fixed number of times for
> EAGAIN errno.
[AndersW] The call to select() above should wait until either there is
data available for reading or there is an error. So read() should
normally not return EAGAIN here. I know there is a possibility that
select() may claim the file descriptor is readable but then a subsequent
read() still returns EAGAIN (or blocks), but I think that can only
happen with sockets used for network connections and not for pipes.
So I don't think it is necessary to check for EAGAIN here. If the code
is to be made more generic and not rely on the fact that the file
descriptor refers to a pipe, then yes it would be necessary. In that
case there is also another bug: the code doesn't handle short reads. In
general, read may return fewer bytes than we requested, so we would need
keep track of how many bytes have been read so far, and loop to read the
rest of them. But for pipes, read() and write() are guaranteed to be
atomic for sizes up to the page size, so it is not necessary here.
Maybe we could consider in the future to add a new utility function to
the library: an osaf_read() function with an (optional) time-out, that
handles both EINTR and short reads. That function could then be used
here and probably at other places as well.
>> }
>> while (read(filedes[0], &tmp_pid, sizeof(int)) < 0)
>> @@ -1002,7 +1007,7 @@ uint32_t spawn_wait(NID_SPAWN_INFO *serv
>> LOG_ER("Timed-out for response from %s",
>> service->serv_name);
>> return NCSCC_RC_FAILURE;
>> }
>> -
>> + break;
>> }
>> /* Read the message from FIFO and fill in structure. */
>
------------------------------------------------------------------------------
Sponsored by Intel(R) XDK
Develop, test and display web and hybrid apps with a single code base.
Download it for free now!
http://pubads.g.doubleclick.net/gampad/clk?id=111408631&iu=/4140/ostg.clktrk
_______________________________________________
Opensaf-devel mailing list
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel
