Re: [Spice-devel] [PATCH] common: add backtrace via gstack

[Christophe Fergeau]

On Tue, Jul 19, 2011 at 02:22:14PM +0300, Uri Lublin wrote:
> I guess I misunderstood what is suggested to be checked in configure.ac
> From the two options below I meant the second.
> 
> Checking that backtrace or backtrace_symbols are available during
> compile time should be done in configure.ac.
> 
> Checking /usr/bin/gstack existence should not be done at compile
> time and not in configure.ac, but at run-time in the code itself.

Ah, I was talking about the former. And I agree with the latter :)

Christophe


pgpPb7ZWPDMpN.pgp
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH] common: add backtrace via gstack

[Uri Lublin]

On 07/18/2011 01:32 PM, Christophe Fergeau wrote:

On Mon, Jul 18, 2011 at 01:19:05PM +0300, Uri Lublin wrote:

On Mon, Jul 18, 2011 at 11:13:50AM +0200, Christophe Fergeau wrote:

Or this can be checked for in configure.ac


I'm not sure configure.ac is the place to check it.


Why not? If you want to check if a function is available or not on the
system you're building on before using it, configure.ac is a good place for
that.


I guess I misunderstood what is suggested to be checked in configure.ac
From the two options below I meant the second.

Checking that backtrace or backtrace_symbols are available during compile time 
should be done in configure.ac.


Checking /usr/bin/gstack existence should not be done at compile time and not in 
configure.ac, but at run-time in the code itself.

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH] common: add backtrace via gstack

[Christophe Fergeau]

On Mon, Jul 18, 2011 at 01:19:05PM +0300, Uri Lublin wrote:
> >On Mon, Jul 18, 2011 at 11:13:50AM +0200, Christophe Fergeau wrote:
> >>Or this can be checked for in configure.ac
> 
> I'm not sure configure.ac is the place to check it.

Why not? If you want to check if a function is available or not on the
system you're building on before using it, configure.ac is a good place for
that.

Christophe


pgpRhs5QodaXJ.pgp
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH] common: add backtrace via gstack

[Uri Lublin]

On 07/18/2011 12:47 PM, Alon Levy wrote:

On Mon, Jul 18, 2011 at 11:13:50AM +0200, Christophe Fergeau wrote:

On Mon, Jul 18, 2011 at 11:57:47AM +0300, Alon Levy wrote:

On Mon, Jul 18, 2011 at 10:13:27AM +0200, Christophe Fergeau wrote:

What use case do you have in mind for this? Is it meant for use during
development when something asserts but we forgot to run it in gdb? Or do
you want to get more backtraces from crashes in bugzilla? I'm asking
because /usr/bin/gstack comes in the gdb package here, which is not likely
to be installed on "users" boxes. glibc also has backtrace(3) and
backtrace_symbols(3), but they are explicitly documented as GNU extensions.


Both I guess. Ok, so this fails the second use case. I can do both -
compile backtrack support in if it is available (there must be some
define I can use)


Or this can be checked for in configure.ac


I'm not sure configure.ac is the place to check it.





and do gstack first if available, otherwise (if
compiled in) glibc's backtrace.


Yep, though for now we can just go with the gstack based trace, and add the
backtrace() code later if it's really needed.

Christophe


I'll take that as an ack.


Maybe just check (stat) that /usr/bin/gstack exists before fork+exec (although 
it's already safe as in such a scenario exec fails and child exits)


___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH] common: add backtrace via gstack

[Alon Levy]

On Mon, Jul 18, 2011 at 11:13:50AM +0200, Christophe Fergeau wrote:
> On Mon, Jul 18, 2011 at 11:57:47AM +0300, Alon Levy wrote:
> > On Mon, Jul 18, 2011 at 10:13:27AM +0200, Christophe Fergeau wrote:
> > > What use case do you have in mind for this? Is it meant for use during
> > > development when something asserts but we forgot to run it in gdb? Or do
> > > you want to get more backtraces from crashes in bugzilla? I'm asking
> > > because /usr/bin/gstack comes in the gdb package here, which is not likely
> > > to be installed on "users" boxes. glibc also has backtrace(3) and
> > > backtrace_symbols(3), but they are explicitly documented as GNU 
> > > extensions.
> > 
> > Both I guess. Ok, so this fails the second use case. I can do both -
> > compile backtrack support in if it is available (there must be some
> > define I can use)
> 
> Or this can be checked for in configure.ac
> 
> 
> > and do gstack first if available, otherwise (if
> > compiled in) glibc's backtrace.
> 
> Yep, though for now we can just go with the gstack based trace, and add the
> backtrace() code later if it's really needed.
> 
> Christophe

I'll take that as an ack.

> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH] common: add backtrace via gstack

[Christophe Fergeau]

On Mon, Jul 18, 2011 at 11:57:47AM +0300, Alon Levy wrote:
> On Mon, Jul 18, 2011 at 10:13:27AM +0200, Christophe Fergeau wrote:
> > What use case do you have in mind for this? Is it meant for use during
> > development when something asserts but we forgot to run it in gdb? Or do
> > you want to get more backtraces from crashes in bugzilla? I'm asking
> > because /usr/bin/gstack comes in the gdb package here, which is not likely
> > to be installed on "users" boxes. glibc also has backtrace(3) and
> > backtrace_symbols(3), but they are explicitly documented as GNU extensions.
> 
> Both I guess. Ok, so this fails the second use case. I can do both -
> compile backtrack support in if it is available (there must be some
> define I can use)

Or this can be checked for in configure.ac


> and do gstack first if available, otherwise (if
> compiled in) glibc's backtrace.

Yep, though for now we can just go with the gstack based trace, and add the
backtrace() code later if it's really needed.

Christophe


pgp0TY9QhRBWY.pgp
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH] common: add backtrace via gstack

[Alon Levy]

On Mon, Jul 18, 2011 at 10:13:27AM +0200, Christophe Fergeau wrote:
> On Thu, Jul 14, 2011 at 10:50:49AM +0300, Alon Levy wrote:
> > Add a backtrace printing function copied from xserver os/backtrace.c
> > that uses gstack which seems to be available enough that xserver uses it :)
> > Used in ASSERT, tested on F15.
> 
> What use case do you have in mind for this? Is it meant for use during
> development when something asserts but we forgot to run it in gdb? Or do
> you want to get more backtraces from crashes in bugzilla? I'm asking
> because /usr/bin/gstack comes in the gdb package here, which is not likely
> to be installed on "users" boxes. glibc also has backtrace(3) and
> backtrace_symbols(3), but they are explicitly documented as GNU extensions.

Both I guess. Ok, so this fails the second use case. I can do both - compile 
backtrack
support in if it is available (there must be some define I can use) and do 
gstack first
if available, otherwise (if compiled in) glibc's backtrace.

> 
> Apart from this, this is a good idea to have this.
> 
> Christophe


___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH] common: add backtrace via gstack

[Christophe Fergeau]

On Thu, Jul 14, 2011 at 10:50:49AM +0300, Alon Levy wrote:
> Add a backtrace printing function copied from xserver os/backtrace.c
> that uses gstack which seems to be available enough that xserver uses it :)
> Used in ASSERT, tested on F15.

What use case do you have in mind for this? Is it meant for use during
development when something asserts but we forgot to run it in gdb? Or do
you want to get more backtraces from crashes in bugzilla? I'm asking
because /usr/bin/gstack comes in the gdb package here, which is not likely
to be installed on "users" boxes. glibc also has backtrace(3) and
backtrace_symbols(3), but they are explicitly documented as GNU extensions.

Apart from this, this is a good idea to have this.

Christophe


pgpPiYMwtWroT.pgp
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH] common: add backtrace via gstack

[Alon Levy]

On Thu, Jul 14, 2011 at 11:53:30AM +0300, Uri Lublin wrote:
> On 07/14/2011 10:50 AM, Alon Levy wrote:
> >Add a backtrace printing function copied from xserver os/backtrace.c
> >that uses gstack which seems to be available enough that xserver uses it :)
> >Used in ASSERT, tested on F15.
> >---
> >diff --git a/common/backtrace.c b/common/backtrace.c
> >+
> >+while (!done) {
> >+bytesread = read(pipefd[0], btline, sizeof(btline) - 1);
> >+
> >+if (bytesread>  0) {
> >+btline[bytesread] = 0;
> >+red_printf("%s", btline);
> 
> nitpick: why not just printf similarly to ASSERT below ?

right, should be printf. But just because red_printf adds a prefix.

> 
> >+}
> >+else if ((bytesread<  0) ||
> >+ ((errno != EINTR)&&  (errno != EAGAIN)))
> >+done = 1;
> 
> This seems wrong. If read() fails due to EINTR or EAGAIN this
> condition breaks out of the while loop.
> I think it should be
> ((bytesread < 0) && (errno != EINTR) && (errno != EAGAIN))
> 
> or (break when read() returns the value 0 -- end of file)
> ((bytesread == 0) || ((errno != EINTR)&&  (errno != EAGAIN)))
> 

You're right. I think both would work, the second is nicer.

> >+}
> >diff --git a/common/spice_common.h b/common/spice_common.h
> >
> >  #define ASSERT(x) if (!(x)) {   \
> >  printf("%s: ASSERT %s failed\n", __FUNCTION__, #x); \
> >+backtrace();   \
> >  abort();\
> >  }
> >
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH] common: add backtrace via gstack

[Uri Lublin]

On 07/14/2011 10:50 AM, Alon Levy wrote:

Add a backtrace printing function copied from xserver os/backtrace.c
that uses gstack which seems to be available enough that xserver uses it :)
Used in ASSERT, tested on F15.
---
diff --git a/common/backtrace.c b/common/backtrace.c
+
+while (!done) {
+bytesread = read(pipefd[0], btline, sizeof(btline) - 1);
+
+if (bytesread>  0) {
+btline[bytesread] = 0;
+red_printf("%s", btline);


nitpick: why not just printf similarly to ASSERT below ?


+}
+else if ((bytesread<  0) ||
+ ((errno != EINTR)&&  (errno != EAGAIN)))
+done = 1;


This seems wrong. If read() fails due to EINTR or EAGAIN this condition breaks 
out of the while loop.

I think it should be
((bytesread < 0) && (errno != EINTR) && (errno != EAGAIN))

or (break when read() returns the value 0 -- end of file)
((bytesread == 0) || ((errno != EINTR)&&  (errno != EAGAIN)))


+}
diff --git a/common/spice_common.h b/common/spice_common.h

  #define ASSERT(x) if (!(x)) {   \
  printf("%s: ASSERT %s failed\n", __FUNCTION__, #x); \
+backtrace();   \
  abort();\
  }



___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH] common: add backtrace via gstack

[Alon Levy]

Add a backtrace printing function copied from xserver os/backtrace.c
that uses gstack which seems to be available enough that xserver uses it :)
Used in ASSERT, tested on F15.
---
 common/Makefile.am|2 +
 common/backtrace.c|   88 +
 common/backtrace.h|   24 +
 common/spice_common.h |2 +
 4 files changed, 116 insertions(+), 0 deletions(-)
 create mode 100644 common/backtrace.c
 create mode 100644 common/backtrace.h

diff --git a/common/Makefile.am b/common/Makefile.am
index e0f4d49..f07f948 100644
--- a/common/Makefile.am
+++ b/common/Makefile.am
@@ -36,6 +36,8 @@ libspice_common_la_SOURCES =  \
spice_common.h  \
ssl_verify.c\
ssl_verify.h\
+   backtrace.c \
+   backtrace.h \
$(NULL)
 
 if SUPPORT_GL
diff --git a/common/backtrace.c b/common/backtrace.c
new file mode 100644
index 000..658b6b0
--- /dev/null
+++ b/common/backtrace.c
@@ -0,0 +1,88 @@
+/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */
+/*
+   Copyright (C) 2011 Red Hat, Inc.
+
+   This library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   This library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with this library; if not, see .
+*/
+
+/*
+ * Taken from xserver os/backtrace.c:
+ * Copyright 2008 Red Hat, Inc.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include "common/spice_common.h"
+
+static int backtrace_gstack(void) {
+pid_t kidpid;
+int pipefd[2];
+
+if (pipe(pipefd) != 0) {
+return -1;
+}
+
+kidpid = fork();
+
+if (kidpid == -1) {
+/* ERROR */
+return -1;
+} else if (kidpid == 0) {
+/* CHILD */
+char parent[16];
+
+seteuid(0);
+close(STDIN_FILENO);
+close(STDOUT_FILENO);
+dup2(pipefd[1],STDOUT_FILENO);
+close(STDERR_FILENO);
+
+snprintf(parent, sizeof(parent), "%d", getppid());
+execle("/usr/bin/gstack", "gstack", parent, NULL, NULL);
+exit(1);
+} else {
+/* PARENT */
+char btline[256];
+int kidstat;
+int bytesread;
+int done = 0;
+
+close(pipefd[1]);
+
+while (!done) {
+bytesread = read(pipefd[0], btline, sizeof(btline) - 1);
+
+if (bytesread > 0) {
+btline[bytesread] = 0;
+red_printf("%s", btline);
+}
+else if ((bytesread < 0) ||
+ ((errno != EINTR) && (errno != EAGAIN)))
+done = 1;
+}
+close(pipefd[0]);
+waitpid(kidpid, &kidstat, 0);
+if (kidstat != 0)
+return -1;
+}
+return 0;
+}
+
+void backtrace() {
+backtrace_gstack(); /* no implementation other then via gstack */
+}
diff --git a/common/backtrace.h b/common/backtrace.h
new file mode 100644
index 000..83a54a1
--- /dev/null
+++ b/common/backtrace.h
@@ -0,0 +1,24 @@
+/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */
+/*
+   Copyright (C) 2011 Red Hat, Inc.
+
+   This library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   This library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with this library; if not, see .
+*/
+
+#ifndef BACKTRACE_H
+#define BACKTRACE_H
+
+void backtrace(void);
+
+#endif // BACKTRACE_H
diff --git a/common/spice_common.h b/common/spice_common.h
index bc74486..d58c558 100644
--- a/common/spice_common.h
+++ b/common/spice_common.h
@@ -22,9 +22,11 @@
 #include 
 #include 
 #include 
+#include "backtrace.h"
 
 #define ASSERT(x) if (!(x)) {   \
 printf("%s: ASSERT %s failed\n", __FUNCTION__, #x); \
+backtrace();   \
 abort();\
 }
 
-- 
1.7.6

___