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@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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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)
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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@laine.org> -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.