Re: [libvirt] [PATCH REPOST 04/38] virlog: Export virLogOutputPtr through header

On 10/05/16 02:08, Cole Robinson wrote: > On 05/04/2016 10:30 AM, Erik Skultety wrote: >> It needs to be exported, since some caller might (for some reason) want to >> create a logging output without calling the parser which does this. Also, >> some methods will use virLogOutputPtr as data type for one of its arguments. >> --- >> src/util/virlog.c | 2 -- >> src/util/virlog.h | 3 +++ >> 2 files changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/src/util/virlog.c b/src/util/virlog.c >> index 812e2cd..0be1701 100644 >> --- a/src/util/virlog.c >> +++ b/src/util/virlog.c >> @@ -106,8 +106,6 @@ struct _virLogOutput { >> virLogDestination dest; >> char *name; >> }; >> -typedef struct _virLogOutput virLogOutput; >> -typedef virLogOutput *virLogOutputPtr; >> >> static virLogOutputPtr *virLogOutputs; >> static size_t virLogNbOutputs; >> diff --git a/src/util/virlog.h b/src/util/virlog.h >> index b5056f5..7706d22 100644 >> --- a/src/util/virlog.h >> +++ b/src/util/virlog.h >> @@ -130,6 +130,9 @@ struct _virLogMetadata { >> typedef struct _virLogMetadata virLogMetadata; >> typedef struct _virLogMetadata *virLogMetadataPtr; >> >> +typedef struct _virLogOutput virLogOutput; >> +typedef virLogOutput *virLogOutputPtr; >> + >> /** >> * virLogOutputFunc: >> * @src: the source of the log message >> > > ACK, but IMO exporting it early in a separate patch without context makes it > hard to follow the reasoning. Better would have been to export it with the > first public function that needs it, looks like virLogDefineOutputs > > - Cole > I tried to break all the changes into as many bits as possible, so that it could be reviewed more easily and I did it with the best intentions, but I have to admit that you're absolutely right and without any context, this can be very hard to follow for a reviewer. Also, for the sake of commit history, I think squashing this change into the commit where I'm introducing virLogDefineOutputs might be a better choice than pushing this as a standalone patch. Analogically, I'll squash 5/38 into the commit which introduces virLogDefineFilters. Erik