[libvirt] [PATCH v2 08/20] virlog: Take a special care of syslog when setting new set of log outputs

[John Ferlan]

On 08/18/2016 07:47 AM, Erik Skultety wrote:
> Now that we're in the critical section, syslog connection can be re-opened
> by issuing openlog, which is something that cannot be done beforehand, since
> syslog keeps its file descriptor private and changing the tag earlier might
> introduce a log inconsistency if something went wrong with preparing a new set
> of logging outputs in order to replace the existing one.
> 
> Signed-off-by: Erik Skultety <eskultet at redhat.com>
> ---
>  src/util/virlog.c | 23 +++++++++++++++++++++--
>  1 file changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/src/util/virlog.c b/src/util/virlog.c
> index c0b8b9a..61e71a3 100644
> --- a/src/util/virlog.c
> +++ b/src/util/virlog.c
> @@ -1794,16 +1794,35 @@ virLogFindOutput(virLogOutputPtr *outputs, size_t noutputs,
>  int
>  virLogDefineOutputs(virLogOutputPtr *outputs, size_t noutputs)
>  {
> +    int ret = -1;
> +    size_t i;
> +    char *tmp = NULL;
> +
>      if (virLogInitialize() < 0)
>          return -1;
>  
>      virLogLock();
>      virLogResetOutputs();
> +
> +    /* nothing can go wrong now (except for malloc) and we're also holding the
> +     * lock so it's safe to call openlog and change the message tag */
> +    for (i = 0; i < noutputs; i++) {
> +        if (outputs[i]->dest == VIR_LOG_TO_SYSLOG) {

Is this essentially open coded virLogFindOutput?  Or is it that the
->name and current_ident check that would trip that up based on the
weirdness from the previous patch?

If you modify the Find to do that STREQ_NULLABLE check - I think then
that function could be used...


> +            if (VIR_STRDUP_QUIET(tmp, outputs[i]->name) < 0)
> +                goto cleanup;
> +            VIR_FREE(current_ident);
> +            current_ident = tmp;
> +            openlog(current_ident, 0, 0);
> +        }
> +    }
> +
>      virLogOutputs = outputs;
>      virLogNbOutputs = noutputs;
> -    virLogUnlock();
>  
> -    return virLogNbOutputs;
> +    ret = virLogNbOutputs;

BTW: Still see no reason to not have ret be 0... From the earlier comment...

Conditional ACK... curious about usage of virFindLogOutput...

John

> + cleanup:
> +    virLogUnlock();
> +    return ret;
>  }
>  
>  
>