8:33 a.m.
Remove a few unnecessary copies of the JSON string as well as duplicate
and unneeded debug logs.
Peter Krempa (7):
util: buffer: Remove struct member munging
util: buffer: Use 'size_t' for buffer size variables
util: json: Use virBuffer in JSON->string conversion
util: json: Don't bother logging output string in virJSONValueToString
util: json: Export virJSONValueToBuffer
qemu: monitor: Remove few debug statements
qemu: monitor: Avoid unnecessary copies of command string
src/libvirt_private.syms | 1 +
src/qemu/qemu_monitor_json.c | 19 ++++++++-----------
src/util/virbuffer.c | 14 +-------------
src/util/virbuffer.h | 18 +++++++-----------
src/util/virjson.c | 33 ++++++++++++++++++++++++---------
src/util/virjson.h | 5 +++++
tests/virbuftest.c | 4 +++-
7 files changed, 49 insertions(+), 45 deletions(-)
--
2.20.1
8:33 a.m.
New subject: [libvirt] [PATCH 1/7] util: buffer: Remove struct member munging
This was meant to stop abusing the members directly, but we don't do
this for other internal structs. Additionally this did not stop the
test from touching the members. Remove the header obscurization.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/util/virbuffer.c | 12 ------------
src/util/virbuffer.h | 16 ++++++----------
tests/virbuftest.c | 2 +-
3 files changed, 7 insertions(+), 23 deletions(-)
diff --git a/src/util/virbuffer.c b/src/util/virbuffer.c
index 12bdd13d39..8bb9c8e1fa 100644
--- a/src/util/virbuffer.c
+++ b/src/util/virbuffer.c
@@ -23,24 +23,12 @@
#include <stdarg.h>
#include "c-ctype.h"
-#define __VIR_BUFFER_C__
-
#include "virbuffer.h"
#include "virerror.h"
#include "virstring.h"
#define VIR_FROM_THIS VIR_FROM_NONE
-/* If adding more fields, ensure to edit buf.h to match
- the number of fields */
-struct _virBuffer {
- unsigned int size;
- unsigned int use;
- unsigned int error; /* errno value, or -1 for usage error */
- int indent;
- char *content;
-};
-
/**
* virBufferFail
* @buf: the buffer
diff --git a/src/util/virbuffer.h b/src/util/virbuffer.h
index b399c90154..16cd8515d6 100644
--- a/src/util/virbuffer.h
+++ b/src/util/virbuffer.h
@@ -35,19 +35,15 @@
typedef struct _virBuffer virBuffer;
typedef virBuffer *virBufferPtr;
-# ifndef __VIR_BUFFER_C__
-# define VIR_BUFFER_INITIALIZER { 0, 0, 0, 0, NULL }
+# define VIR_BUFFER_INITIALIZER { 0, 0, 0, 0, NULL }
-/* This struct must be kept in sync with the real struct
- in the buf.c impl file */
struct _virBuffer {
- unsigned int a;
- unsigned int b;
- unsigned int c;
- int d;
- char *e;
+ unsigned int size;
+ unsigned int use;
+ unsigned int error; /* errno value, or -1 for usage error */
+ int indent;
+ char *content;
};
-# endif
const char *virBufferCurrentContent(virBufferPtr buf);
char *virBufferContentAndReset(virBufferPtr buf);
diff --git a/tests/virbuftest.c b/tests/virbuftest.c
index 34f02b1281..b608da94d4 100644
--- a/tests/virbuftest.c
+++ b/tests/virbuftest.c
@@ -29,7 +29,7 @@ static int testBufInfiniteLoop(const void *data)
* which was the case after the above addchar at the time of the bug.
* This test is a bit fragile, since it relies on virBuffer internals.
*/
- if (virAsprintf(&addstr, "%*s", buf->a - buf->b - 1,
"a") < 0)
+ if (virAsprintf(&addstr, "%*s", buf->size - buf->use - 1,
"a") < 0)
goto out;
if (info->doEscape)
--
2.20.1
5:44 p.m.
New subject: [libvirt] [PATCH 1/7] util: buffer: Remove struct member munging
On 3/29/19 9:33 AM, Peter Krempa wrote:
This was meant to stop abusing the members directly, but we don't
do
this for other internal structs. Additionally this did not stop the
test from touching the members. Remove the header obscurization.
I agree with you that this obfuscation does nothing in the face of a
hostile (or ignorant) coder, but if we instead just make the real struct
public then it won't be just ignorant devs who incorrectly use the
internals of virBuffer. How about taking care of it with a
virbufferpriv.h as we now do for several other structs whose internals
we want to keep "private"? vircommandpriv.h is one good example.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/util/virbuffer.c | 12 ------------
src/util/virbuffer.h | 16 ++++++----------
tests/virbuftest.c | 2 +-
3 files changed, 7 insertions(+), 23 deletions(-)
diff --git a/src/util/virbuffer.c b/src/util/virbuffer.c
index 12bdd13d39..8bb9c8e1fa 100644
--- a/src/util/virbuffer.c
+++ b/src/util/virbuffer.c
@@ -23,24 +23,12 @@
#include <stdarg.h>
#include "c-ctype.h"
-#define __VIR_BUFFER_C__
-
#include "virbuffer.h"
#include "virerror.h"
#include "virstring.h"
#define VIR_FROM_THIS VIR_FROM_NONE
-/* If adding more fields, ensure to edit buf.h to match
- the number of fields */
-struct _virBuffer {
- unsigned int size;
- unsigned int use;
- unsigned int error; /* errno value, or -1 for usage error */
- int indent;
- char *content;
-};
-
/**
* virBufferFail
* @buf: the buffer
diff --git a/src/util/virbuffer.h b/src/util/virbuffer.h
index b399c90154..16cd8515d6 100644
--- a/src/util/virbuffer.h
+++ b/src/util/virbuffer.h
@@ -35,19 +35,15 @@
typedef struct _virBuffer virBuffer;
typedef virBuffer *virBufferPtr;
-# ifndef __VIR_BUFFER_C__
-# define VIR_BUFFER_INITIALIZER { 0, 0, 0, 0, NULL }
+# define VIR_BUFFER_INITIALIZER { 0, 0, 0, 0, NULL }
-/* This struct must be kept in sync with the real struct
- in the buf.c impl file */
struct _virBuffer {
- unsigned int a;
- unsigned int b;
- unsigned int c;
- int d;
- char *e;
+ unsigned int size;
+ unsigned int use;
+ unsigned int error; /* errno value, or -1 for usage error */
+ int indent;
+ char *content;
};
-# endif
const char *virBufferCurrentContent(virBufferPtr buf);
char *virBufferContentAndReset(virBufferPtr buf);
diff --git a/tests/virbuftest.c b/tests/virbuftest.c
index 34f02b1281..b608da94d4 100644
--- a/tests/virbuftest.c
+++ b/tests/virbuftest.c
@@ -29,7 +29,7 @@ static int testBufInfiniteLoop(const void *data)
* which was the case after the above addchar at the time of the bug.
* This test is a bit fragile, since it relies on virBuffer internals.
*/
- if (virAsprintf(&addstr, "%*s", buf->a - buf->b - 1,
"a") < 0)
+ if (virAsprintf(&addstr, "%*s", buf->size - buf->use - 1,
"a") < 0)
goto out;
if (info->doEscape)
1:54 a.m.
New subject: [libvirt] [PATCH 1/7] util: buffer: Remove struct member munging
On Fri, Mar 29, 2019 at 18:44:04 -0400, Laine Stump wrote:
On 3/29/19 9:33 AM, Peter Krempa wrote:
> This was meant to stop abusing the members directly, but we don't do
> this for other internal structs. Additionally this did not stop the
> test from touching the members. Remove the header obscurization.
I agree with you that this obfuscation does nothing in the face of a hostile
(or ignorant) coder, but if we instead just make the real struct public then
it won't be just ignorant devs who incorrectly use the internals of
virBuffer. How about taking care of it with a virbufferpriv.h as we now do
for several other structs whose internals we want to keep "private"?
vircommandpriv.h is one good example.
Vast majority of our code uses stack'd version. Just grep for use of the
VIR_BUFFER_INITIALIZER macro. If it were possible I'd take it private.
I think review making sure that people don't do shenaningans with the
struct should be sufficient.
5:42 p.m.
New subject: [libvirt] [PATCH 1/7] util: buffer: Remove struct member munging
On 4/1/19 2:54 AM, Peter Krempa wrote:
On Fri, Mar 29, 2019 at 18:44:04 -0400, Laine Stump wrote:
> On 3/29/19 9:33 AM, Peter Krempa wrote:
>> This was meant to stop abusing the members directly, but we don't do
>> this for other internal structs. Additionally this did not stop the
>> test from touching the members. Remove the header obscurization.
>
> I agree with you that this obfuscation does nothing in the face of a hostile
> (or ignorant) coder, but if we instead just make the real struct public then
> it won't be just ignorant devs who incorrectly use the internals of
> virBuffer. How about taking care of it with a virbufferpriv.h as we now do
> for several other structs whose internals we want to keep "private"?
> vircommandpriv.h is one good example.
Vast majority of our code uses stack'd version. Just grep for use of the
VIR_BUFFER_INITIALIZER macro.
Ah, right. I hadn't noticed that.
If it were possible I'd take it private.
Personally I think it's easy to go overboard making things private
(although I have to admit it does occasionally make it easier to change
the implementation of something without requiring little changes all
over the place).
I think review making sure that people don't do shenaningans with
the
struct should be sufficient.
My only misgiving is that there must have been some event leading up to
commit 642b26fab26 back in 2008.
Personally, I'm okay with it. You may want to wait and see if danpb (the
author of the above commit) thinks differently:
Reviewed-by: Laine Stump <laine(a)laine.org>
8:33 a.m.
New subject: [libvirt] [PATCH 2/7] util: buffer: Use 'size_t' for buffer size variables
Use size_t for all sizes. The '*' modifier unfortunately does require an
int so a temporary variable is necessary in the tests.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/util/virbuffer.c | 2 +-
src/util/virbuffer.h | 6 +++---
tests/virbuftest.c | 4 +++-
3 files changed, 7 insertions(+), 5 deletions(-)
diff --git a/src/util/virbuffer.c b/src/util/virbuffer.c
index 8bb9c8e1fa..2e1e4abead 100644
--- a/src/util/virbuffer.c
+++ b/src/util/virbuffer.c
@@ -339,7 +339,7 @@ virBufferCheckErrorInternal(const virBuffer *buf,
*
* Return the string usage in bytes
*/
-unsigned int
+size_t
virBufferUse(const virBuffer *buf)
{
if (buf == NULL)
diff --git a/src/util/virbuffer.h b/src/util/virbuffer.h
index 16cd8515d6..18957ae02c 100644
--- a/src/util/virbuffer.h
+++ b/src/util/virbuffer.h
@@ -38,8 +38,8 @@ typedef virBuffer *virBufferPtr;
# define VIR_BUFFER_INITIALIZER { 0, 0, 0, 0, NULL }
struct _virBuffer {
- unsigned int size;
- unsigned int use;
+ size_t size;
+ size_t use;
unsigned int error; /* errno value, or -1 for usage error */
int indent;
char *content;
@@ -69,7 +69,7 @@ VIR_DEFINE_AUTOCLEAN_FUNC(virBuffer, virBufferFreeAndReset);
# define virBufferCheckError(buf) \
virBufferCheckErrorInternal(buf, VIR_FROM_THIS, __FILE__, __FUNCTION__, \
__LINE__)
-unsigned int virBufferUse(const virBuffer *buf);
+size_t virBufferUse(const virBuffer *buf);
void virBufferAdd(virBufferPtr buf, const char *str, int len);
void virBufferAddBuffer(virBufferPtr buf, virBufferPtr toadd);
void virBufferAddChar(virBufferPtr buf, char c);
diff --git a/tests/virbuftest.c b/tests/virbuftest.c
index b608da94d4..778754d7c1 100644
--- a/tests/virbuftest.c
+++ b/tests/virbuftest.c
@@ -20,6 +20,7 @@ static int testBufInfiniteLoop(const void *data)
char *addstr = NULL, *bufret = NULL;
int ret = -1;
const struct testInfo *info = data;
+ int len;
virBufferAddChar(buf, 'a');
@@ -29,7 +30,8 @@ static int testBufInfiniteLoop(const void *data)
* which was the case after the above addchar at the time of the bug.
* This test is a bit fragile, since it relies on virBuffer internals.
*/
- if (virAsprintf(&addstr, "%*s", buf->size - buf->use - 1,
"a") < 0)
+ len = buf->size - buf->use - 1;
+ if (virAsprintf(&addstr, "%*s", len, "a") < 0)
goto out;
if (info->doEscape)
--
2.20.1
11:21 a.m.
New subject: [libvirt] [PATCH 2/7] util: buffer: Use 'size_t' for buffer size variables
On 3/29/19 9:33 AM, Peter Krempa wrote:
Use size_t for all sizes. The '*' modifier unfortunately does
require an
int so a temporary variable is necessary in the tests.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/util/virbuffer.c | 2 +-
src/util/virbuffer.h | 6 +++---
tests/virbuftest.c | 4 +++-
3 files changed, 7 insertions(+), 5 deletions(-)
diff --git a/src/util/virbuffer.c b/src/util/virbuffer.c
index 8bb9c8e1fa..2e1e4abead 100644
--- a/src/util/virbuffer.c
+++ b/src/util/virbuffer.c
@@ -339,7 +339,7 @@ virBufferCheckErrorInternal(const virBuffer *buf,
*
* Return the string usage in bytes
*/
-unsigned int
+size_t
virBufferUse(const virBuffer *buf)
{
if (buf == NULL)
diff --git a/src/util/virbuffer.h b/src/util/virbuffer.h
index 16cd8515d6..18957ae02c 100644
--- a/src/util/virbuffer.h
+++ b/src/util/virbuffer.h
@@ -38,8 +38,8 @@ typedef virBuffer *virBufferPtr;
# define VIR_BUFFER_INITIALIZER { 0, 0, 0, 0, NULL }
struct _virBuffer {
- unsigned int size;
- unsigned int use;
+ size_t size;
+ size_t use;
unsigned int error; /* errno value, or -1 for usage error */
int indent;
char *content;
@@ -69,7 +69,7 @@ VIR_DEFINE_AUTOCLEAN_FUNC(virBuffer, virBufferFreeAndReset);
# define virBufferCheckError(buf) \
virBufferCheckErrorInternal(buf, VIR_FROM_THIS, __FILE__, __FUNCTION__, \
__LINE__)
-unsigned int virBufferUse(const virBuffer *buf);
+size_t virBufferUse(const virBuffer *buf);
void virBufferAdd(virBufferPtr buf, const char *str, int len);
void virBufferAddBuffer(virBufferPtr buf, virBufferPtr toadd);
void virBufferAddChar(virBufferPtr buf, char c);
diff --git a/tests/virbuftest.c b/tests/virbuftest.c
index b608da94d4..778754d7c1 100644
--- a/tests/virbuftest.c
+++ b/tests/virbuftest.c
@@ -20,6 +20,7 @@ static int testBufInfiniteLoop(const void *data)
char *addstr = NULL, *bufret = NULL;
int ret = -1;
const struct testInfo *info = data;
+ int len;
virBufferAddChar(buf, 'a');
@@ -29,7 +30,8 @@ static int testBufInfiniteLoop(const void *data)
* which was the case after the above addchar at the time of the bug.
* This test is a bit fragile, since it relies on virBuffer internals.
*/
- if (virAsprintf(&addstr, "%*s", buf->size - buf->use - 1,
"a") < 0)
+ len = buf->size - buf->use - 1;
+ if (virAsprintf(&addstr, "%*s", len, "a") < 0)
If you really wanted to avoid the temporary int (which you've implied
you don't like in the commit message), you could just do a typecast in
the arg list.
Reviewed-by: Laine Stump <laine(a)laine.org>
goto out;
if (info->doEscape)
1:59 a.m.
New subject: [libvirt] [PATCH 2/7] util: buffer: Use 'size_t' for buffer size variables
On Sat, Mar 30, 2019 at 12:21:51 -0400, Laine Stump wrote:
On 3/29/19 9:33 AM, Peter Krempa wrote:
> Use size_t for all sizes. The '*' modifier unfortunately does require an
> int so a temporary variable is necessary in the tests.
>
> Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
> ---
> src/util/virbuffer.c | 2 +-
> src/util/virbuffer.h | 6 +++---
> tests/virbuftest.c | 4 +++-
> 3 files changed, 7 insertions(+), 5 deletions(-)
[...]
> @@ -29,7 +30,8 @@ static int testBufInfiniteLoop(const void
*data)
> * which was the case after the above addchar at the time of the bug.
> * This test is a bit fragile, since it relies on virBuffer internals.
> */
> - if (virAsprintf(&addstr, "%*s", buf->size - buf->use - 1,
"a") < 0)
> + len = buf->size - buf->use - 1;
> + if (virAsprintf(&addstr, "%*s", len, "a") < 0)
If you really wanted to avoid the temporary int (which you've implied you
don't like in the commit message), you could just do a typecast in the arg
list.
That looke worse.
5:44 p.m.
New subject: [libvirt] [PATCH 2/7] util: buffer: Use 'size_t' for buffer size variables
On 4/1/19 2:59 AM, Peter Krempa wrote:
On Sat, Mar 30, 2019 at 12:21:51 -0400, Laine Stump wrote:
> On 3/29/19 9:33 AM, Peter Krempa wrote:
>> Use size_t for all sizes. The '*' modifier unfortunately does require an
>> int so a temporary variable is necessary in the tests.
>>
>> Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
>> ---
>> src/util/virbuffer.c | 2 +-
>> src/util/virbuffer.h | 6 +++---
>> tests/virbuftest.c | 4 +++-
>> 3 files changed, 7 insertions(+), 5 deletions(-)
[...]
>> @@ -29,7 +30,8 @@ static int testBufInfiniteLoop(const void *data)
>> * which was the case after the above addchar at the time of the bug.
>> * This test is a bit fragile, since it relies on virBuffer internals.
>> */
>> - if (virAsprintf(&addstr, "%*s", buf->size - buf->use -
1, "a") < 0)
>> + len = buf->size - buf->use - 1;
>> + if (virAsprintf(&addstr, "%*s", len, "a") < 0)
>
> If you really wanted to avoid the temporary int (which you've implied you
> don't like in the commit message), you could just do a typecast in the arg
> list.
That looke worse.
Agreed. It just sounded like maybe you regretted putting in the int, so
I was offering you an out :-)
8:33 a.m.
New subject: [libvirt] [PATCH 3/7] util: json: Use virBuffer in JSON->string conversion
The last step of the conversion involves copying of the generated JSON
into a separate string. We can use a virBuffer to do this as this will
also allow to subsequently use the buffer when we actually need to do
some other formatting of the string.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/util/virjson.c | 38 +++++++++++++++++++++++++++++---------
1 file changed, 29 insertions(+), 9 deletions(-)
diff --git a/src/util/virjson.c b/src/util/virjson.c
index d5d66f879f..7dfc589944 100644
--- a/src/util/virjson.c
+++ b/src/util/virjson.c
@@ -28,6 +28,7 @@
#include "virlog.h"
#include "virstring.h"
#include "virutil.h"
+#include "virbuffer.h"
#if WITH_YAJL
# include <yajl/yajl_gen.h>
@@ -1969,17 +1970,18 @@ virJSONValueToStringOne(virJSONValuePtr object,
}
-char *
-virJSONValueToString(virJSONValuePtr object,
+static int
+virJSONValueToBuffer(virJSONValuePtr object,
+ virBufferPtr buf,
bool pretty)
{
yajl_gen g;
const unsigned char *str;
- char *ret = NULL;
yajl_size_t len;
# ifndef WITH_YAJL2
yajl_gen_config conf = { pretty ? 1 : 0, pretty ? " " : " "};
# endif
+ int ret = -1;
VIR_DEBUG("object=%p", object);
@@ -2009,13 +2011,12 @@ virJSONValueToString(virJSONValuePtr object,
goto cleanup;
}
- ignore_value(VIR_STRDUP(ret, (const char *)str));
+ virBufferAdd(buf, (const char *) str, len);
+ ret = 0;
cleanup:
yajl_gen_free(g);
- VIR_DEBUG("result=%s", NULLSTR(ret));
-
return ret;
}
@@ -2030,17 +2031,36 @@ virJSONValueFromString(const char *jsonstring ATTRIBUTE_UNUSED)
}
-char *
-virJSONValueToString(virJSONValuePtr object ATTRIBUTE_UNUSED,
+static int
+virJSONValueToBuffer(virJSONValuePtr object ATTRIBUTE_UNUSED,
+ virBufferPtr buf ATTRIBUTE_UNUSED,
bool pretty ATTRIBUTE_UNUSED)
{
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("No JSON parser implementation is available"));
- return NULL;
+ return -1;
}
#endif
+char *
+virJSONValueToString(virJSONValuePtr object,
+ bool pretty)
+{
+ VIR_AUTOCLEAN(virBuffer) buf = VIR_BUFFER_INITIALIZER;
+ char *ret = NULL;
+
+ if (virJSONValueToBuffer(object, &buf, pretty) < 0)
+ return NULL;
+
+ ret = virBufferContentAndReset(&buf);
+
+ VIR_DEBUG("result=%s", NULLSTR(ret));
+
+ return ret;
+}
+
+
/**
* virJSONStringReformat:
* @jsonstr: string to reformat
--
2.20.1
7:23 p.m.
New subject: [libvirt] [PATCH 3/7] util: json: Use virBuffer in JSON->string conversion
On 3/29/19 9:33 AM, Peter Krempa wrote:
The last step of the conversion involves copying of the generated
JSON
into a separate string. We can use a virBuffer to do this as this will
also allow to subsequently use the buffer when we actually need to do
some other formatting of the string.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
Reviewed-by: Laine Stump <laine(a)laine.org>
(by itself it's pointless to convert. But of course in patch 7 you're
using the ToBuffer function separately)
---
src/util/virjson.c | 38 +++++++++++++++++++++++++++++---------
1 file changed, 29 insertions(+), 9 deletions(-)
diff --git a/src/util/virjson.c b/src/util/virjson.c
index d5d66f879f..7dfc589944 100644
--- a/src/util/virjson.c
+++ b/src/util/virjson.c
@@ -28,6 +28,7 @@
#include "virlog.h"
#include "virstring.h"
#include "virutil.h"
+#include "virbuffer.h"
#if WITH_YAJL
# include <yajl/yajl_gen.h>
@@ -1969,17 +1970,18 @@ virJSONValueToStringOne(virJSONValuePtr object,
}
-char *
-virJSONValueToString(virJSONValuePtr object,
+static int
+virJSONValueToBuffer(virJSONValuePtr object,
+ virBufferPtr buf,
bool pretty)
{
yajl_gen g;
const unsigned char *str;
- char *ret = NULL;
yajl_size_t len;
# ifndef WITH_YAJL2
yajl_gen_config conf = { pretty ? 1 : 0, pretty ? " " : " "};
# endif
+ int ret = -1;
VIR_DEBUG("object=%p", object);
@@ -2009,13 +2011,12 @@ virJSONValueToString(virJSONValuePtr object,
goto cleanup;
}
- ignore_value(VIR_STRDUP(ret, (const char *)str));
+ virBufferAdd(buf, (const char *) str, len);
+ ret = 0;
cleanup:
yajl_gen_free(g);
- VIR_DEBUG("result=%s", NULLSTR(ret));
-
return ret;
}
@@ -2030,17 +2031,36 @@ virJSONValueFromString(const char *jsonstring ATTRIBUTE_UNUSED)
}
-char *
-virJSONValueToString(virJSONValuePtr object ATTRIBUTE_UNUSED,
+static int
+virJSONValueToBuffer(virJSONValuePtr object ATTRIBUTE_UNUSED,
+ virBufferPtr buf ATTRIBUTE_UNUSED,
bool pretty ATTRIBUTE_UNUSED)
{
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("No JSON parser implementation is available"));
- return NULL;
+ return -1;
}
#endif
+char *
+virJSONValueToString(virJSONValuePtr object,
+ bool pretty)
+{
+ VIR_AUTOCLEAN(virBuffer) buf = VIR_BUFFER_INITIALIZER;
+ char *ret = NULL;
+
+ if (virJSONValueToBuffer(object, &buf, pretty) < 0)
+ return NULL;
+
+ ret = virBufferContentAndReset(&buf);
+
+ VIR_DEBUG("result=%s", NULLSTR(ret));
+
+ return ret;
+}
+
+
/**
* virJSONStringReformat:
* @jsonstr: string to reformat
8:33 a.m.
New subject: [libvirt] [PATCH 4/7] util: json: Don't bother logging output string in virJSONValueToString
We have tests that validate the XML formatter. Additionally almost every
guide tells users to disable JSON logging. Drop logging of output string
in virJSONValueToString.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/util/virjson.c | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/src/util/virjson.c b/src/util/virjson.c
index 7dfc589944..19857d2f2f 100644
--- a/src/util/virjson.c
+++ b/src/util/virjson.c
@@ -2048,16 +2048,11 @@ virJSONValueToString(virJSONValuePtr object,
bool pretty)
{
VIR_AUTOCLEAN(virBuffer) buf = VIR_BUFFER_INITIALIZER;
- char *ret = NULL;
if (virJSONValueToBuffer(object, &buf, pretty) < 0)
return NULL;
- ret = virBufferContentAndReset(&buf);
-
- VIR_DEBUG("result=%s", NULLSTR(ret));
-
- return ret;
+ return virBufferContentAndReset(&buf);
}
--
2.20.1
7:22 p.m.
New subject: [libvirt] [PATCH 4/7] util: json: Don't bother logging output string in virJSONValueToString
On 3/29/19 9:33 AM, Peter Krempa wrote:
We have tests that validate the XML formatter. Additionally almost
every
guide tells users to disable JSON logging. Drop logging of output string
in virJSONValueToString.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
Assuming that anywhere it might be useful to see the json string in the
debug log, it's being debug-logged by the caller of this function...
Reviewed-by: Laine Stump <laine(a)laine.org>
---
src/util/virjson.c | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/src/util/virjson.c b/src/util/virjson.c
index 7dfc589944..19857d2f2f 100644
--- a/src/util/virjson.c
+++ b/src/util/virjson.c
@@ -2048,16 +2048,11 @@ virJSONValueToString(virJSONValuePtr object,
bool pretty)
{
VIR_AUTOCLEAN(virBuffer) buf = VIR_BUFFER_INITIALIZER;
- char *ret = NULL;
if (virJSONValueToBuffer(object, &buf, pretty) < 0)
return NULL;
- ret = virBufferContentAndReset(&buf);
-
- VIR_DEBUG("result=%s", NULLSTR(ret));
-
- return ret;
+ return virBufferContentAndReset(&buf);
}
8:33 a.m.
New subject: [libvirt] [PATCH 5/7] util: json: Export virJSONValueToBuffer
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/libvirt_private.syms | 1 +
src/util/virjson.c | 4 ++--
src/util/virjson.h | 5 +++++
3 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 73ef24d66f..7b9ea23ab9 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2202,6 +2202,7 @@ virJSONValueObjectKeysNumber;
virJSONValueObjectRemoveKey;
virJSONValueObjectStealArray;
virJSONValueObjectStealObject;
+virJSONValueToBuffer;
virJSONValueToString;
diff --git a/src/util/virjson.c b/src/util/virjson.c
index 19857d2f2f..c519f8139e 100644
--- a/src/util/virjson.c
+++ b/src/util/virjson.c
@@ -1970,7 +1970,7 @@ virJSONValueToStringOne(virJSONValuePtr object,
}
-static int
+int
virJSONValueToBuffer(virJSONValuePtr object,
virBufferPtr buf,
bool pretty)
@@ -2031,7 +2031,7 @@ virJSONValueFromString(const char *jsonstring ATTRIBUTE_UNUSED)
}
-static int
+int
virJSONValueToBuffer(virJSONValuePtr object ATTRIBUTE_UNUSED,
virBufferPtr buf ATTRIBUTE_UNUSED,
bool pretty ATTRIBUTE_UNUSED)
diff --git a/src/util/virjson.h b/src/util/virjson.h
index 3dee103aba..ec86603794 100644
--- a/src/util/virjson.h
+++ b/src/util/virjson.h
@@ -26,6 +26,7 @@
# include "internal.h"
# include "virbitmap.h"
# include "viralloc.h"
+# include "virbuffer.h"
# include <stdarg.h>
@@ -143,6 +144,10 @@ int virJSONValueArrayAppendString(virJSONValuePtr object, const char
*value);
virJSONValuePtr virJSONValueFromString(const char *jsonstring);
char *virJSONValueToString(virJSONValuePtr object,
bool pretty);
+int virJSONValueToBuffer(virJSONValuePtr object,
+ virBufferPtr buf,
+ bool pretty)
+ ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
typedef int (*virJSONValueObjectIteratorFunc)(const char *key,
virJSONValuePtr value,
--
2.20.1
7:25 p.m.
New subject: [libvirt] [PATCH 5/7] util: json: Export virJSONValueToBuffer
On 3/29/19 9:33 AM, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
I would have just done this directly in Patch 4, but no harm in
separating it.
Reviewed-by: Laine Stump <laine(a)laine.org>
---
src/libvirt_private.syms | 1 +
src/util/virjson.c | 4 ++--
src/util/virjson.h | 5 +++++
3 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 73ef24d66f..7b9ea23ab9 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2202,6 +2202,7 @@ virJSONValueObjectKeysNumber;
virJSONValueObjectRemoveKey;
virJSONValueObjectStealArray;
virJSONValueObjectStealObject;
+virJSONValueToBuffer;
virJSONValueToString;
diff --git a/src/util/virjson.c b/src/util/virjson.c
index 19857d2f2f..c519f8139e 100644
--- a/src/util/virjson.c
+++ b/src/util/virjson.c
@@ -1970,7 +1970,7 @@ virJSONValueToStringOne(virJSONValuePtr object,
}
-static int
+int
virJSONValueToBuffer(virJSONValuePtr object,
virBufferPtr buf,
bool pretty)
@@ -2031,7 +2031,7 @@ virJSONValueFromString(const char *jsonstring ATTRIBUTE_UNUSED)
}
-static int
+int
virJSONValueToBuffer(virJSONValuePtr object ATTRIBUTE_UNUSED,
virBufferPtr buf ATTRIBUTE_UNUSED,
bool pretty ATTRIBUTE_UNUSED)
diff --git a/src/util/virjson.h b/src/util/virjson.h
index 3dee103aba..ec86603794 100644
--- a/src/util/virjson.h
+++ b/src/util/virjson.h
@@ -26,6 +26,7 @@
# include "internal.h"
# include "virbitmap.h"
# include "viralloc.h"
+# include "virbuffer.h"
# include <stdarg.h>
@@ -143,6 +144,10 @@ int virJSONValueArrayAppendString(virJSONValuePtr object, const char
*value);
virJSONValuePtr virJSONValueFromString(const char *jsonstring);
char *virJSONValueToString(virJSONValuePtr object,
bool pretty);
+int virJSONValueToBuffer(virJSONValuePtr object,
+ virBufferPtr buf,
+ bool pretty)
+ ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
typedef int (*virJSONValueObjectIteratorFunc)(const char *key,
virJSONValuePtr value,
8:33 a.m.
New subject: [libvirt] [PATCH 6/7] qemu: monitor: Remove few debug statements
The internal qemu machinery already logs the sent message via the PROBE
point in qemuMonitorSend and the monitor receive function. Those are way
better as they are easy grepable. Remove the additional ones from the
monitor code which just duplicate the sent data.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/qemu/qemu_monitor_json.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 743a88b914..c7a7e3fa56 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -301,14 +301,8 @@ qemuMonitorJSONCommandWithFd(qemuMonitorPtr mon,
msg.txLength = strlen(msg.txBuffer);
msg.txFD = scm_fd;
- VIR_DEBUG("Send command '%s' for write with FD %d", cmdstr,
scm_fd);
-
ret = qemuMonitorSend(mon, &msg);
- VIR_DEBUG("Receive command reply ret=%d rxObject=%p",
- ret, msg.rxObject);
-
-
if (ret == 0) {
if (!msg.rxObject) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
--
2.20.1
7:29 p.m.
New subject: [libvirt] [PATCH 6/7] qemu: monitor: Remove few debug statements
On 3/29/19 9:33 AM, Peter Krempa wrote:
The internal qemu machinery already logs the sent message via the
PROBE
point in qemuMonitorSend and the monitor receive function. Those are way
better as they are easy grepable. Remove the additional ones from the
monitor code which just duplicate the sent data.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
There's a lot of very basic stuff like this that was probably useful to
log when the code was initially being written, but is now just creating
more chaff in the debug logs.
Reviewed-by: Laine Stump <laine(a)laine.org>
---
src/qemu/qemu_monitor_json.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 743a88b914..c7a7e3fa56 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -301,14 +301,8 @@ qemuMonitorJSONCommandWithFd(qemuMonitorPtr mon,
msg.txLength = strlen(msg.txBuffer);
msg.txFD = scm_fd;
- VIR_DEBUG("Send command '%s' for write with FD %d", cmdstr,
scm_fd);
-
ret = qemuMonitorSend(mon, &msg);
- VIR_DEBUG("Receive command reply ret=%d rxObject=%p",
- ret, msg.rxObject);
-
-
if (ret == 0) {
if (!msg.rxObject) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
8:33 a.m.
New subject: [libvirt] [PATCH 7/7] qemu: monitor: Avoid unnecessary copies of command string
Use virJSONValueToBuffer so that we can append the command terminator
string without copying of the string again. Also avoid a 'strlen' as we
can query the buffer use size.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/qemu/qemu_monitor_json.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index c7a7e3fa56..8e6c3ccd63 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -277,7 +277,7 @@ qemuMonitorJSONCommandWithFd(qemuMonitorPtr mon,
{
int ret = -1;
qemuMonitorMessage msg;
- char *cmdstr = NULL;
+ VIR_AUTOCLEAN(virBuffer) cmdbuf = VIR_BUFFER_INITIALIZER;
char *id = NULL;
*reply = NULL;
@@ -294,11 +294,15 @@ qemuMonitorJSONCommandWithFd(qemuMonitorPtr mon,
}
}
- if (!(cmdstr = virJSONValueToString(cmd, false)))
+ if (virJSONValueToBuffer(cmd, &cmdbuf, false) < 0)
goto cleanup;
- if (virAsprintf(&msg.txBuffer, "%s\r\n", cmdstr) < 0)
+ virBufferAddLit(&cmdbuf, "\r\n");
+
+ if (virBufferCheckError(&cmdbuf) < 0)
goto cleanup;
- msg.txLength = strlen(msg.txBuffer);
+
+ msg.txLength = virBufferUse(&cmdbuf);
+ msg.txBuffer = virBufferContentAndReset(&cmdbuf);
msg.txFD = scm_fd;
ret = qemuMonitorSend(mon, &msg);
@@ -315,7 +319,6 @@ qemuMonitorJSONCommandWithFd(qemuMonitorPtr mon,
cleanup:
VIR_FREE(id);
- VIR_FREE(cmdstr);
VIR_FREE(msg.txBuffer);
return ret;
--
2.20.1
7:35 p.m.
New subject: [libvirt] [PATCH 7/7] qemu: monitor: Avoid unnecessary copies of command string
On 3/29/19 9:33 AM, Peter Krempa wrote:
Use virJSONValueToBuffer so that we can append the command
terminator
string without copying of the string again. Also avoid a 'strlen' as we
can query the buffer use size.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
Reviewed-by: Laine Stump <laine(a)laine.org>
---
src/qemu/qemu_monitor_json.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index c7a7e3fa56..8e6c3ccd63 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -277,7 +277,7 @@ qemuMonitorJSONCommandWithFd(qemuMonitorPtr mon,
{
int ret = -1;
qemuMonitorMessage msg;
- char *cmdstr = NULL;
+ VIR_AUTOCLEAN(virBuffer) cmdbuf = VIR_BUFFER_INITIALIZER;
char *id = NULL;
*reply = NULL;
@@ -294,11 +294,15 @@ qemuMonitorJSONCommandWithFd(qemuMonitorPtr mon,
}
}
- if (!(cmdstr = virJSONValueToString(cmd, false)))
+ if (virJSONValueToBuffer(cmd, &cmdbuf, false) < 0)
goto cleanup;
- if (virAsprintf(&msg.txBuffer, "%s\r\n", cmdstr) < 0)
+ virBufferAddLit(&cmdbuf, "\r\n");
+
+ if (virBufferCheckError(&cmdbuf) < 0)
goto cleanup;
- msg.txLength = strlen(msg.txBuffer);
+
+ msg.txLength = virBufferUse(&cmdbuf);
+ msg.txBuffer = virBufferContentAndReset(&cmdbuf);
msg.txFD = scm_fd;
ret = qemuMonitorSend(mon, &msg);
@@ -315,7 +319,6 @@ qemuMonitorJSONCommandWithFd(qemuMonitorPtr mon,
cleanup:
VIR_FREE(id);
- VIR_FREE(cmdstr);
VIR_FREE(msg.txBuffer);
return ret;
2060
days inactive
2065
days old
18 comments
2 participants
participants (2)
-
Laine Stump -
Peter Krempa