Re: [Xen-devel] [PATCH for-4.6] xenstored: log tdb message via xenstored's logging mechanisms
On Thu, Jan 08, 2015 at 01:10:38PM +, Ian Campbell wrote:
> On Thu, 2015-01-08 at 13:04 +, Ian Campbell wrote:
> > On Thu, 2015-01-08 at 12:57 +, Wei Liu wrote:
> > > On Thu, Jan 08, 2015 at 12:33:29PM +, Ian Campbell wrote:
> > > > ping.
> > > >
> > > > On Mon, 2014-12-15 at 13:18 +, Ian Campbell wrote:
> > > > > TDB provides us with a callback for this purpose. Use it in both
> > > > > xenstored and xs_tdb_dump.
> > > > >
> > > > > While at it make the existing log() macro tollerate memory failures.
> > > > >
> > > > > Signed-off-by: Ian Campbell
> > > > > ---
> > > > > tools/xenstore/xenstored_core.c | 39
> > > > > +--
> > > > > tools/xenstore/xs_tdb_dump.c| 12 +++-
> > > > > 2 files changed, 44 insertions(+), 7 deletions(-)
> > > > >
> > > > > diff --git a/tools/xenstore/xenstored_core.c
> > > > > b/tools/xenstore/xenstored_core.c
> > > > > index 4eaff57..3fd9a20 100644
> > > > > --- a/tools/xenstore/xenstored_core.c
> > > > > +++ b/tools/xenstore/xenstored_core.c
> > > > > @@ -89,9 +89,14 @@ static void check_store(void);
> > > > > #define log(...)
> > > > > \
> > > > > do {
> > > > > \
> > > > > char *s = talloc_asprintf(NULL, __VA_ARGS__);
> > > > > \
> > > > > - trace("%s\n", s);
> > > > > \
> > > > > - syslog(LOG_ERR, "%s", s);
> > > > > \
> > > > > - talloc_free(s);
> > > > > \
> > > > > + if (s) {
> > > > > \
> > > > > + trace("%s\n", s);
> > > > > \
> > > > > + syslog(LOG_ERR, "%s", s);
> > > > > \
> > > > > + talloc_free(s);
> > > > > \
> > > > > + } else {
> > > > > \
> > > > > + trace("talloc failure during logging\n");
> > > > > \
> > > > > + syslog(LOG_ERR, "talloc failure during
> > > > > logging\n"); \
> > > > > + }
> > > > > \
> > >
> > > talloc_free can tolerate NULL pointer.
> >
> > Good point, I'll fix.
>
> Hrm, it tolerates but it does return failure (-1) instead of success in
> that case.
>
> Literally nowhere in our code base actually checks the return value from
> talloc_free, but nonetheless given that I think it would prefer to keep
> the talloc_free where it is and therefore only pass a valid pointer to
> it.
>
Fair enough.
Wei.
> Ian.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH for-4.6] xenstored: log tdb message via xenstored's logging mechanisms
On Thu, 2015-01-08 at 13:04 +, Ian Campbell wrote:
> On Thu, 2015-01-08 at 12:57 +, Wei Liu wrote:
> > On Thu, Jan 08, 2015 at 12:33:29PM +, Ian Campbell wrote:
> > > ping.
> > >
> > > On Mon, 2014-12-15 at 13:18 +, Ian Campbell wrote:
> > > > TDB provides us with a callback for this purpose. Use it in both
> > > > xenstored and xs_tdb_dump.
> > > >
> > > > While at it make the existing log() macro tollerate memory failures.
> > > >
> > > > Signed-off-by: Ian Campbell
> > > > ---
> > > > tools/xenstore/xenstored_core.c | 39
> > > > +--
> > > > tools/xenstore/xs_tdb_dump.c| 12 +++-
> > > > 2 files changed, 44 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/tools/xenstore/xenstored_core.c
> > > > b/tools/xenstore/xenstored_core.c
> > > > index 4eaff57..3fd9a20 100644
> > > > --- a/tools/xenstore/xenstored_core.c
> > > > +++ b/tools/xenstore/xenstored_core.c
> > > > @@ -89,9 +89,14 @@ static void check_store(void);
> > > > #define log(...)
> > > > \
> > > > do {
> > > > \
> > > > char *s = talloc_asprintf(NULL, __VA_ARGS__);
> > > > \
> > > > - trace("%s\n", s);
> > > > \
> > > > - syslog(LOG_ERR, "%s", s);
> > > > \
> > > > - talloc_free(s);
> > > > \
> > > > + if (s) {
> > > > \
> > > > + trace("%s\n", s);
> > > > \
> > > > + syslog(LOG_ERR, "%s", s);
> > > > \
> > > > + talloc_free(s);
> > > > \
> > > > + } else {
> > > > \
> > > > + trace("talloc failure during logging\n");
> > > > \
> > > > + syslog(LOG_ERR, "talloc failure during
> > > > logging\n"); \
> > > > + }
> > > > \
> >
> > talloc_free can tolerate NULL pointer.
>
> Good point, I'll fix.
Hrm, it tolerates but it does return failure (-1) instead of success in
that case.
Literally nowhere in our code base actually checks the return value from
talloc_free, but nonetheless given that I think it would prefer to keep
the talloc_free where it is and therefore only pass a valid pointer to
it.
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH for-4.6] xenstored: log tdb message via xenstored's logging mechanisms
On Thu, Jan 08, 2015 at 01:04:26PM +, Ian Campbell wrote:
> On Thu, 2015-01-08 at 12:57 +, Wei Liu wrote:
> > On Thu, Jan 08, 2015 at 12:33:29PM +, Ian Campbell wrote:
> > > ping.
> > >
> > > On Mon, 2014-12-15 at 13:18 +, Ian Campbell wrote:
> > > > TDB provides us with a callback for this purpose. Use it in both
> > > > xenstored and xs_tdb_dump.
> > > >
> > > > While at it make the existing log() macro tollerate memory failures.
> > > >
> > > > Signed-off-by: Ian Campbell
> > > > ---
> > > > tools/xenstore/xenstored_core.c | 39
> > > > +--
> > > > tools/xenstore/xs_tdb_dump.c| 12 +++-
> > > > 2 files changed, 44 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/tools/xenstore/xenstored_core.c
> > > > b/tools/xenstore/xenstored_core.c
> > > > index 4eaff57..3fd9a20 100644
> > > > --- a/tools/xenstore/xenstored_core.c
> > > > +++ b/tools/xenstore/xenstored_core.c
> > > > @@ -89,9 +89,14 @@ static void check_store(void);
> > > > #define log(...)
> > > > \
> > > > do {
> > > > \
> > > > char *s = talloc_asprintf(NULL, __VA_ARGS__);
> > > > \
> > > > - trace("%s\n", s);
> > > > \
> > > > - syslog(LOG_ERR, "%s", s);
> > > > \
> > > > - talloc_free(s);
> > > > \
> > > > + if (s) {
> > > > \
> > > > + trace("%s\n", s);
> > > > \
> > > > + syslog(LOG_ERR, "%s", s);
> > > > \
> > > > + talloc_free(s);
> > > > \
> > > > + } else {
> > > > \
> > > > + trace("talloc failure during logging\n");
> > > > \
> > > > + syslog(LOG_ERR, "talloc failure during
> > > > logging\n"); \
> > > > + }
> > > > \
> >
> > talloc_free can tolerate NULL pointer.
>
> Good point, I'll fix. I think I'll also change the message to more
> clearly indicate an allocation failure, since that is what has actually
> happened.
>
> > > > } while (0)
> > > >
> > [...]
> > > > + tdb_ctx = tdb_open_ex(tdbname, 0, tdb_flags, O_RDWR, 0,
> > > > + &tdb_logger, NULL);
> > [...]
> > > > + tdb_ctx = tdb_open_ex(tdbname, 7919, tdb_flags,
> > > > O_RDWR|O_CREAT,
> > > > + 0640, &tdb_logger, NULL);
> > [...]
> > > > + tdb = tdb_open_ex(talloc_strdup(NULL, argv[1]), 0, 0, O_RDONLY,
> > > > 0,
> > > > + tdb_logger, NULL);
> >
> > Should be &tdb_logger?
>
> tdb_logger is a function, so it's not strictly required, but it is good
> form (and a hint to the reader) plus inconsistent with the others, so
> I'll change.
>
Yeah. I was more annoyed by the different styles in used. :-)
Wei.
> Ian.
>
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH for-4.6] xenstored: log tdb message via xenstored's logging mechanisms
On Thu, 2015-01-08 at 12:57 +, Wei Liu wrote:
> On Thu, Jan 08, 2015 at 12:33:29PM +, Ian Campbell wrote:
> > ping.
> >
> > On Mon, 2014-12-15 at 13:18 +, Ian Campbell wrote:
> > > TDB provides us with a callback for this purpose. Use it in both
> > > xenstored and xs_tdb_dump.
> > >
> > > While at it make the existing log() macro tollerate memory failures.
> > >
> > > Signed-off-by: Ian Campbell
> > > ---
> > > tools/xenstore/xenstored_core.c | 39
> > > +--
> > > tools/xenstore/xs_tdb_dump.c| 12 +++-
> > > 2 files changed, 44 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/tools/xenstore/xenstored_core.c
> > > b/tools/xenstore/xenstored_core.c
> > > index 4eaff57..3fd9a20 100644
> > > --- a/tools/xenstore/xenstored_core.c
> > > +++ b/tools/xenstore/xenstored_core.c
> > > @@ -89,9 +89,14 @@ static void check_store(void);
> > > #define log(...) \
> > > do {\
> > > char *s = talloc_asprintf(NULL, __VA_ARGS__); \
> > > - trace("%s\n", s); \
> > > - syslog(LOG_ERR, "%s", s); \
> > > - talloc_free(s); \
> > > + if (s) {\
> > > + trace("%s\n", s); \
> > > + syslog(LOG_ERR, "%s", s); \
> > > + talloc_free(s); \
> > > + } else {\
> > > + trace("talloc failure during logging\n"); \
> > > + syslog(LOG_ERR, "talloc failure during logging\n"); \
> > > + } \
>
> talloc_free can tolerate NULL pointer.
Good point, I'll fix. I think I'll also change the message to more
clearly indicate an allocation failure, since that is what has actually
happened.
> > > } while (0)
> > >
> [...]
> > > + tdb_ctx = tdb_open_ex(tdbname, 0, tdb_flags, O_RDWR, 0,
> > > + &tdb_logger, NULL);
> [...]
> > > + tdb_ctx = tdb_open_ex(tdbname, 7919, tdb_flags, O_RDWR|O_CREAT,
> > > + 0640, &tdb_logger, NULL);
> [...]
> > > + tdb = tdb_open_ex(talloc_strdup(NULL, argv[1]), 0, 0, O_RDONLY, 0,
> > > + tdb_logger, NULL);
>
> Should be &tdb_logger?
tdb_logger is a function, so it's not strictly required, but it is good
form (and a hint to the reader) plus inconsistent with the others, so
I'll change.
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH for-4.6] xenstored: log tdb message via xenstored's logging mechanisms
On Thu, Jan 08, 2015 at 12:33:29PM +, Ian Campbell wrote:
> ping.
>
> On Mon, 2014-12-15 at 13:18 +, Ian Campbell wrote:
> > TDB provides us with a callback for this purpose. Use it in both
> > xenstored and xs_tdb_dump.
> >
> > While at it make the existing log() macro tollerate memory failures.
> >
> > Signed-off-by: Ian Campbell
> > ---
> > tools/xenstore/xenstored_core.c | 39
> > +--
> > tools/xenstore/xs_tdb_dump.c| 12 +++-
> > 2 files changed, 44 insertions(+), 7 deletions(-)
> >
> > diff --git a/tools/xenstore/xenstored_core.c
> > b/tools/xenstore/xenstored_core.c
> > index 4eaff57..3fd9a20 100644
> > --- a/tools/xenstore/xenstored_core.c
> > +++ b/tools/xenstore/xenstored_core.c
> > @@ -89,9 +89,14 @@ static void check_store(void);
> > #define log(...) \
> > do {\
> > char *s = talloc_asprintf(NULL, __VA_ARGS__); \
> > - trace("%s\n", s); \
> > - syslog(LOG_ERR, "%s", s); \
> > - talloc_free(s); \
> > + if (s) {\
> > + trace("%s\n", s); \
> > + syslog(LOG_ERR, "%s", s); \
> > + talloc_free(s); \
> > + } else {\
> > + trace("talloc failure during logging\n"); \
> > + syslog(LOG_ERR, "talloc failure during logging\n"); \
> > + } \
talloc_free can tolerate NULL pointer.
> > } while (0)
> >
[...]
> > + tdb_ctx = tdb_open_ex(tdbname, 0, tdb_flags, O_RDWR, 0,
> > + &tdb_logger, NULL);
[...]
> > + tdb_ctx = tdb_open_ex(tdbname, 7919, tdb_flags, O_RDWR|O_CREAT,
> > + 0640, &tdb_logger, NULL);
[...]
> > + tdb = tdb_open_ex(talloc_strdup(NULL, argv[1]), 0, 0, O_RDONLY, 0,
> > + tdb_logger, NULL);
Should be &tdb_logger?
Wei.
> > if (!tdb)
> > barf_perror("Could not open %s", argv[1]);
> >
>
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH for-4.6] xenstored: log tdb message via xenstored's logging mechanisms
ping.
On Mon, 2014-12-15 at 13:18 +, Ian Campbell wrote:
> TDB provides us with a callback for this purpose. Use it in both
> xenstored and xs_tdb_dump.
>
> While at it make the existing log() macro tollerate memory failures.
>
> Signed-off-by: Ian Campbell
> ---
> tools/xenstore/xenstored_core.c | 39
> +--
> tools/xenstore/xs_tdb_dump.c| 12 +++-
> 2 files changed, 44 insertions(+), 7 deletions(-)
>
> diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
> index 4eaff57..3fd9a20 100644
> --- a/tools/xenstore/xenstored_core.c
> +++ b/tools/xenstore/xenstored_core.c
> @@ -89,9 +89,14 @@ static void check_store(void);
> #define log(...) \
> do {\
> char *s = talloc_asprintf(NULL, __VA_ARGS__); \
> - trace("%s\n", s); \
> - syslog(LOG_ERR, "%s", s); \
> - talloc_free(s); \
> + if (s) {\
> + trace("%s\n", s); \
> + syslog(LOG_ERR, "%s", s); \
> + talloc_free(s); \
> + } else {\
> + trace("talloc failure during logging\n"); \
> + syslog(LOG_ERR, "talloc failure during logging\n"); \
> + } \
> } while (0)
>
>
> @@ -1479,13 +1484,35 @@ static void manual_node(const char *name, const char
> *child)
> talloc_free(node);
> }
>
> +static void tdb_logger(TDB_CONTEXT *tdb, int level, const char * fmt, ...)
> +{
> + va_list ap;
> + char *s;
> +
> + va_start(ap, fmt);
> + s = talloc_vasprintf(NULL, fmt, ap);
> + va_end(ap);
> +
> + if (s) {
> + trace("TDB: %s\n", s);
> + syslog(LOG_ERR, "TDB: %s", s);
> + if (verbose)
> + xprintf("TDB: %s", s);
> + talloc_free(s);
> + } else {
> + trace("talloc failure during logging\n");
> + syslog(LOG_ERR, "talloc failure during logging\n");
> + }
> +}
> +
> static void setup_structure(void)
> {
> char *tdbname;
> tdbname = talloc_strdup(talloc_autofree_context(), xs_daemon_tdb());
>
> if (!(tdb_flags & TDB_INTERNAL))
> - tdb_ctx = tdb_open(tdbname, 0, tdb_flags, O_RDWR, 0);
> + tdb_ctx = tdb_open_ex(tdbname, 0, tdb_flags, O_RDWR, 0,
> + &tdb_logger, NULL);
>
> if (tdb_ctx) {
> /* XXX When we make xenstored able to restart, this will have
> @@ -1516,8 +1543,8 @@ static void setup_structure(void)
> talloc_free(tlocal);
> }
> else {
> - tdb_ctx = tdb_open(tdbname, 7919, tdb_flags, O_RDWR|O_CREAT,
> -0640);
> + tdb_ctx = tdb_open_ex(tdbname, 7919, tdb_flags, O_RDWR|O_CREAT,
> + 0640, &tdb_logger, NULL);
> if (!tdb_ctx)
> barf_perror("Could not create tdb file %s", tdbname);
>
> diff --git a/tools/xenstore/xs_tdb_dump.c b/tools/xenstore/xs_tdb_dump.c
> index b91cdef..7a034c0 100644
> --- a/tools/xenstore/xs_tdb_dump.c
> +++ b/tools/xenstore/xs_tdb_dump.c
> @@ -33,6 +33,15 @@ static char perm_to_char(enum xs_perm_type perm)
> '?';
> }
>
> +static void tdb_logger(TDB_CONTEXT *tdb, int level, const char * fmt, ...)
> +{
> + va_list ap;
> +
> + va_start(ap, fmt);
> + vfprintf(stderr, fmt, ap);
> + va_end(ap);
> +}
> +
> int main(int argc, char *argv[])
> {
> TDB_DATA key;
> @@ -41,7 +50,8 @@ int main(int argc, char *argv[])
> if (argc != 2)
> barf("Usage: xs_tdb_dump ");
>
> - tdb = tdb_open(talloc_strdup(NULL, argv[1]), 0, 0, O_RDONLY, 0);
> + tdb = tdb_open_ex(talloc_strdup(NULL, argv[1]), 0, 0, O_RDONLY, 0,
> + tdb_logger, NULL);
> if (!tdb)
> barf_perror("Could not open %s", argv[1]);
>
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
[Xen-devel] [PATCH for-4.6] xenstored: log tdb message via xenstored's logging mechanisms
TDB provides us with a callback for this purpose. Use it in both
xenstored and xs_tdb_dump.
While at it make the existing log() macro tollerate memory failures.
Signed-off-by: Ian Campbell
---
tools/xenstore/xenstored_core.c | 39 +--
tools/xenstore/xs_tdb_dump.c| 12 +++-
2 files changed, 44 insertions(+), 7 deletions(-)
diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index 4eaff57..3fd9a20 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -89,9 +89,14 @@ static void check_store(void);
#define log(...) \
do {\
char *s = talloc_asprintf(NULL, __VA_ARGS__); \
- trace("%s\n", s); \
- syslog(LOG_ERR, "%s", s); \
- talloc_free(s); \
+ if (s) {\
+ trace("%s\n", s); \
+ syslog(LOG_ERR, "%s", s); \
+ talloc_free(s); \
+ } else {\
+ trace("talloc failure during logging\n"); \
+ syslog(LOG_ERR, "talloc failure during logging\n"); \
+ } \
} while (0)
@@ -1479,13 +1484,35 @@ static void manual_node(const char *name, const char
*child)
talloc_free(node);
}
+static void tdb_logger(TDB_CONTEXT *tdb, int level, const char * fmt, ...)
+{
+ va_list ap;
+ char *s;
+
+ va_start(ap, fmt);
+ s = talloc_vasprintf(NULL, fmt, ap);
+ va_end(ap);
+
+ if (s) {
+ trace("TDB: %s\n", s);
+ syslog(LOG_ERR, "TDB: %s", s);
+ if (verbose)
+ xprintf("TDB: %s", s);
+ talloc_free(s);
+ } else {
+ trace("talloc failure during logging\n");
+ syslog(LOG_ERR, "talloc failure during logging\n");
+ }
+}
+
static void setup_structure(void)
{
char *tdbname;
tdbname = talloc_strdup(talloc_autofree_context(), xs_daemon_tdb());
if (!(tdb_flags & TDB_INTERNAL))
- tdb_ctx = tdb_open(tdbname, 0, tdb_flags, O_RDWR, 0);
+ tdb_ctx = tdb_open_ex(tdbname, 0, tdb_flags, O_RDWR, 0,
+ &tdb_logger, NULL);
if (tdb_ctx) {
/* XXX When we make xenstored able to restart, this will have
@@ -1516,8 +1543,8 @@ static void setup_structure(void)
talloc_free(tlocal);
}
else {
- tdb_ctx = tdb_open(tdbname, 7919, tdb_flags, O_RDWR|O_CREAT,
- 0640);
+ tdb_ctx = tdb_open_ex(tdbname, 7919, tdb_flags, O_RDWR|O_CREAT,
+ 0640, &tdb_logger, NULL);
if (!tdb_ctx)
barf_perror("Could not create tdb file %s", tdbname);
diff --git a/tools/xenstore/xs_tdb_dump.c b/tools/xenstore/xs_tdb_dump.c
index b91cdef..7a034c0 100644
--- a/tools/xenstore/xs_tdb_dump.c
+++ b/tools/xenstore/xs_tdb_dump.c
@@ -33,6 +33,15 @@ static char perm_to_char(enum xs_perm_type perm)
'?';
}
+static void tdb_logger(TDB_CONTEXT *tdb, int level, const char * fmt, ...)
+{
+ va_list ap;
+
+ va_start(ap, fmt);
+ vfprintf(stderr, fmt, ap);
+ va_end(ap);
+}
+
int main(int argc, char *argv[])
{
TDB_DATA key;
@@ -41,7 +50,8 @@ int main(int argc, char *argv[])
if (argc != 2)
barf("Usage: xs_tdb_dump ");
- tdb = tdb_open(talloc_strdup(NULL, argv[1]), 0, 0, O_RDONLY, 0);
+ tdb = tdb_open_ex(talloc_strdup(NULL, argv[1]), 0, 0, O_RDONLY, 0,
+ tdb_logger, NULL);
if (!tdb)
barf_perror("Could not open %s", argv[1]);
--
1.7.10.4
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
