Re: [Xen-changelog] Make xenstored reopen its trace file on SIGHUP. This allows one to rotate the

Re: [Xen-changelog] Make xenstored reopen its trace file on SIGHUP. This allows one to rotate the

[Anthony Liguori]

Xen patchbot -unstable wrote:

> 
>+void reopen_log()
>+{
>+	if (!tracefile)
>+		return;
>+
>+	if (tracefd > 0)
>+		close(tracefd);
>+	tracefd = open(tracefile, O_WRONLY|O_CREAT|O_APPEND, 0600);
>+	if (tracefd < 0) {
>+		perror("Could not open tracefile");
>+		return;
>+	}
>+	write(tracefd, "\n***\n", strlen("\n***\n"));
>+}
>+
>  
>
perror and strlen are not safe to call from a signal handler.

I suggest just removing the perror call altogether and replacing strlen 
with sizeof() - 1.

Regards,

Anthony Liguori

> static bool write_messages(struct connection *conn)
> {
> 	int ret;
>@@ -1498,11 +1514,7 @@
> 			outputpid = true;
> 			break;
> 		case 'T':
>-			tracefd = open(optarg, O_WRONLY|O_CREAT|O_APPEND, 0600);
>-			if (tracefd < 0)
>-				barf_perror("Could not open tracefile %s",
>-					    optarg);
>-                        write(tracefd, "\n***\n", strlen("\n***\n"));
>+			tracefile = optarg;
> 			break;
> 		case 'V':
> 			verbose = true;
>@@ -1511,6 +1523,8 @@
> 	}
> 	if (optind != argc)
> 		barf("%s: No arguments desired", argv[0]);
>+
>+	reopen_log();
> 
> 	if (dofork) {
> 		openlog("xenstored", 0, LOG_DAEMON);
>@@ -1577,6 +1591,8 @@
> 		close(STDOUT_FILENO);
> 		close(STDERR_FILENO);
> 	}
>+
>+	signal(SIGHUP, reopen_log);
> 
> #ifdef TESTING
> 	signal(SIGUSR1, stop_failtest);
>
>_______________________________________________
>Xen-changelog mailing list
>Xen-changelog@lists.xensource.com
>http://lists.xensource.com/xen-changelog
>
>  
>

Re: [Xen-changelog] Make xenstored reopen its trace file on SIGHUP. This allows one to rotate the

[Ewan Mellor]

On Mon, Nov 07, 2005 at 11:15:34AM -0600, Anthony Liguori wrote:

> Xen patchbot -unstable wrote:
> 
> >
> >+void reopen_log()
> >+{
> >+	if (!tracefile)
> >+		return;
> >+
> >+	if (tracefd > 0)
> >+		close(tracefd);
> >+	tracefd = open(tracefile, O_WRONLY|O_CREAT|O_APPEND, 0600);
> >+	if (tracefd < 0) {
> >+		perror("Could not open tracefile");
> >+		return;
> >+	}
> >+	write(tracefd, "\n***\n", strlen("\n***\n"));
> >+}
> >+
> > 
> >
> perror and strlen are not safe to call from a signal handler.

OK, I'll believe you about perror -  thanks for spotting that.

Why, though, should strlen not be safe?  Even if strlen(constant) doesn't turn
into a constant integer at compile-time, which I rather hope that it would,
why in any case would strlen be a problem?

Ewan.

Re: Re: [Xen-changelog] Make xenstored reopen its trace file on SIGHUP. This allows one to rotate the

[Hollis Blanchard]

On Monday 07 November 2005 11:15, Anthony Liguori wrote:
> >+void reopen_log()
> >+{
> >+      if (!tracefile)
> >+              return;
> >+
> >+      if (tracefd > 0)
> >+              close(tracefd);
> >+      tracefd = open(tracefile, O_WRONLY|O_CREAT|O_APPEND, 0600);
> >+      if (tracefd < 0) {
> >+              perror("Could not open tracefile");
> >+              return;
> >+      }
> >+      write(tracefd, "\n***\n", strlen("\n***\n"));
> >+}
>
> perror and strlen are not safe to call from a signal handler.
>
> I suggest just removing the perror call altogether and replacing strlen
> with sizeof() - 1.

I'm really not sure how strlen could be non-threadsafe. I don't care if it's 
not in your list; your list doesn't include strtok_r either, and that is 
explicitly thread-safe. :-P

However, in addition to perror, another thread could try to use tracefd 
between the close and the open. You'd probably want to do something like:
	oldfd = tracefd;
	newfd = open();
	if (newfd < 0) {
		...
	}
	tracefd = newfd;
	close(oldfd);

Finally, you really should do error-checking on close(). See the close(2) man 
page...

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: Re: [Xen-changelog] Make xenstored reopen its trace file on SIGHUP. This allows one to rotate the

[Anthony Liguori]

Hollis Blanchard wrote:

>On Monday 07 November 2005 11:15, Anthony Liguori wrote:
>  
>
>>perror and strlen are not safe to call from a signal handler.
>>
>>I suggest just removing the perror call altogether and replacing strlen
>>with sizeof() - 1.
>>    
>>
>
>I'm really not sure how strlen could be non-threadsafe. I don't care if it's 
>not in your list; your list doesn't include strtok_r either, and that is 
>explicitly thread-safe. :-P
>  
>
So re-reading the manpage, the list of safe functions is a POSIX-ism.  
That's perhaps why the thread-safe string functions aren't there since 
those are defined by ANSI not POSIX.

Regards,

Anthony Liguori

>However, in addition to perror, another thread could try to use tracefd 
>between the close and the open. You'd probably want to do something like:
>	oldfd = tracefd;
>	newfd = open();
>	if (newfd < 0) {
>		...
>	}
>	tracefd = newfd;
>	close(oldfd);
>
>Finally, you really should do error-checking on close(). See the close(2) man 
>page...
>
>  
>