qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 2/2] qemu-ga: Add the guest-suspend command


From: Michael Roth
Subject: Re: [Qemu-devel] [PATCH 2/2] qemu-ga: Add the guest-suspend command
Date: Mon, 16 Jan 2012 16:06:45 -0600
User-agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.24) Gecko/20111109 Thunderbird/3.1.16

On 01/16/2012 02:35 PM, Daniel P. Berrange wrote:
On Mon, Jan 16, 2012 at 02:02:08PM -0600, Michael Roth wrote:
On 01/16/2012 11:23 AM, Luiz Capitulino wrote:
On Mon, 16 Jan 2012 15:18:37 -0200
Luiz Capitulino<address@hidden>   wrote:

On Mon, 16 Jan 2012 17:13:39 +0000
"Daniel P. Berrange"<address@hidden>   wrote:

On Mon, Jan 16, 2012 at 03:08:53PM -0200, Luiz Capitulino wrote:
On Fri, 13 Jan 2012 14:48:04 -0700
Eric Blake<address@hidden>   wrote:

+
+        pid = fork();
+        if (!pid) {
+            char buf[32];
+            FILE *sysfile;
+            const char *arg;
+            const char *pmutils_bin = "pm-is-supported";
+
+            if (strcmp(mode, "hibernate") == 0) {

Strangely enough, POSIX doesn't include strcmp() in its list of
async-signal-safe functions (which is what you should be restricting
yourself to, if qemu-ga is multi-threaded), but in practice, I think
that is a bug of omission in POSIX, and not something you have to change
in your code.

memset() ins't either... sigaction() either, which begins to get
annoying.

For those familiar with glib: isn't it possible to confirm it's using
threads and/or acquire a global mutex or something?

Misread, sigaction() is there. The ones that aren't are strcmp(), strstr()
and memset(). Interestingly, they are all "string functions".


There seem to be things beyond that list required to be implemented
as thread/signal safe:

http://www.unix.org/whitepapers/reentrant.html

fread()/fwrite()/f* for instance, more at `man flockfile`:

        The  stdio  functions are thread-safe.
        This is achieved by assigning to  each
        FILE  object  a  lockcount and (if the
        lockcount  is   nonzero)   an   owning
        thread.   For each library call, these
        functions wait until the  FILE  object
        is  no  longer  locked  by a different
        thread, then lock it, do the requested
        I/O, and unlock the object again.

You need to be careful with terminology here.

    Threadsafe != async signal safe

STDIO is one of the major areas of code that is definitely not
async signal safe. Consider Thread A doing something like
fwrite(stderr, "Foo\n"), while another thread forks, and then
its child also does an fwrite(stderr, "Foo\n"). Given that
every stdio function will lock/unlock a mutex, you easily get
this sequence of events:

1.     Thread A: lock(stderr)
2.     Thread A: write(stderr, "foo\n");
3.     Thread B: fork() ->  Process B1
4.     Thread A: unlock(stderr)
5.   Process B1: lock(stderr)

When the child process is started at step 3, the FILE *stderr
object will be locked by thread A.  When Thread A does the
unlock in step 4, it has no effect on Process B1. So process
B1 hangs forever in step 5.


Ahh, thanks for the example. I missed that these issues were specifically WRT to code that was fork()'d from a multi-threaded application. Seemed pretty scary otherwise :)

In practice, are these functions really a problem for multi-threaded
applications (beyond concurrent access to shared storage)? Maybe it
would be sufficient to just check the glibc sources?

In libvirt we have seen the hang scenarios I describe in the real world.
Causes I rememeber were use of malloc (via asprintf()), or use of stdio
FILE * functions, and use of syslog. The libvirt code still isn't 100%
in compliance with avoiding async signal safe functions, but we have
cleaned up many problems in this area.

Thanks, looks like I have so fixes to do in qmp_guest_shutdown. syslog is really unfortunate...


Regards,
Daniel




reply via email to

[Prev in Thread] Current Thread [Next in Thread]