[libvirt] [PATCH 0/8] Last pile of Coverity changes
After this pile of changes I am back down to zero issues. Patch 1 - is a repeat offender - this is the 3rd try, but I think I have a better solution here. Details are in the commit message. Patch 2-3 - are repeats from the last series as patches 6 and 9. For the virtime.c code - I just removed the offending math and deadcode leaving the entrails in the commit message and code. For the virstoragefile.c code - I agree with eblake's logic and just removed the entire check as dead. Patch 4 - seems to be a real issue. Initially I thought it wasn't, but after finding the original commit it seems it was a copypaste type error. I initially thought perhaps if hard_limit was set as the unlimited value that the design was don't change soft_limit and swap_hard_limit Patch 5 - false positive Patch 6 - false positive mostly, but just add the proper checks like other code Patch 7 - the BAD_SIZEOF was being triggered on the math to the PROBE macro. By generating local variables - the issue went away Patch 8 - initially I wasn't so sure on this - that !!sock_path is just one of those constructs I find "odd" to read in code. Anyway, as it turns out this is a false positive as there is no way sock_path could be null here - so I just added the sa_assert() and that satisfies Coverity John Ferlan (8): remote_driver: Resolve Coverity RESOURCE_LEAK virtime: Resolve Coverity DEADCODE virstoragefile: Resolve Coverity DEADCODE domain_conf: Resolve Coverity COPY_PASTE_ERROR virsh: Resolve Coverity DEADCODE Resolve Coverity CHECKED_RETURN qemu: Resolve Coverity BAD_SIZEOF daemon: Resolve Coverity FORWARD_NULL daemon/libvirtd.c | 1 + src/conf/domain_conf.c | 4 ++-- src/qemu/qemu_monitor.c | 18 +++++++---------- src/remote/remote_driver.c | 49 ++++++++++++++++++++++++++-------------------- src/util/virstoragefile.c | 2 -- src/util/virtime.c | 14 ++++--------- tests/objecteventtest.c | 3 ++- tools/virsh-domain.c | 1 + tools/virt-login-shell.c | 2 +- 9 files changed, 46 insertions(+), 48 deletions(-) -- 1.9.3
Since 98b9acf5aa02551dd37d0209339aba2e22e4004a This was a false positive where Coverity was complaining that the remoteDeserializeTypedParameters() could allocate 'params', but none of the callers could return the allocated memory back to their caller since on input the param was passed by value. Additionally, the flow of the code was that if params was NULL on entry, then each function would return 'nparams' as the number of params entries the caller would need to allocate in order to call the function again with 'nparams' and 'params' being set. By the time the deserialize routine was called params would have something. For other callers where the 'params' was passed by reference as NULL since it's expected that the deserialize allocates the memory and then have that passed back to the original caller to dispose there was no Coverity issue. As it turns out Coverity didn't quite seem to understand the relationship between 'nparams' and 'params'; however, if the !userAllocated path of the deserialize code compared against limit in any manner, then the Coverity error went away which was quite strange, but useful. As it turns out one code path remoteDomainGetJobStats had a comparison against 'limit' while another remoteConnectGetAllDomainStats did not assuming that limit would be checked. So I refactored the code a bit to cause the limit check to occur in deserialize for both conditions and then only made the check of current returned size against the incoming *nparams fail the non allocation case. This means the job code doesn't need to check the limit any more, while the stats code now does check the limit. Additionally, to help perhaps decipher which of the various callers to the deserialize code caused the failure - I used a #define to pass the __FUNCNAME__ of the caller along so that error messages could have something like: error: remoteConnectGetAllDomainStats: too many parameters '2' for nparams '0' error: Reconnected to the hypervisor (it's a contrived error just to show the funcname in the error) Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/remote/remote_driver.c | 49 ++++++++++++++++++++++++++-------------------- 1 file changed, 28 insertions(+), 21 deletions(-) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 8221683..4a1b04b 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -72,6 +72,11 @@ VIR_LOG_INIT("remote.remote_driver"); # define HYPER_TO_ULONG(_to, _from) (_to) = (_from) #endif +#define remoteDeserializeTypedParameters(ret_params_val, ret_params_len, \ + limit, params, nparams) \ + deserializeTypedParameters(__FUNCTION__, ret_params_val, ret_params_len, \ + limit, params, nparams) + static bool inside_daemon = false; static virDriverPtr remoteDriver = NULL; @@ -1743,21 +1748,30 @@ remoteSerializeTypedParameters(virTypedParameterPtr params, /* Helper to deserialize typed parameters. */ static int -remoteDeserializeTypedParameters(remote_typed_param *ret_params_val, - u_int ret_params_len, - int limit, - virTypedParameterPtr *params, - int *nparams) +deserializeTypedParameters(const char *funcname, + remote_typed_param *ret_params_val, + u_int ret_params_len, + int limit, + virTypedParameterPtr *params, + int *nparams) { size_t i = 0; int rv = -1; bool userAllocated = *params != NULL; + if (ret_params_len > limit) { + virReportError(VIR_ERR_RPC, + _("%s: too many parameters '%u' for limit '%d'"), + funcname, ret_params_len, limit); + goto cleanup; + } + if (userAllocated) { /* Check the length of the returned list carefully. */ - if (ret_params_len > limit || ret_params_len > *nparams) { - virReportError(VIR_ERR_RPC, "%s", - _("returned number of parameters exceeds limit")); + if (ret_params_len > *nparams) { + virReportError(VIR_ERR_RPC, + _("%s: too many parameters '%u' for nparams '%d'"), + funcname, ret_params_len, *nparams); goto cleanup; } } else { @@ -1774,8 +1788,8 @@ remoteDeserializeTypedParameters(remote_typed_param *ret_params_val, if (virStrcpyStatic(param->field, ret_param->field) == NULL) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("Parameter %s too big for destination"), - ret_param->field); + _("%s: parameter %s too big for destination"), + funcname, ret_param->field); goto cleanup; } @@ -1811,8 +1825,8 @@ remoteDeserializeTypedParameters(remote_typed_param *ret_params_val, goto cleanup; break; default: - virReportError(VIR_ERR_RPC, _("unknown parameter type: %d"), - param->type); + virReportError(VIR_ERR_RPC, _("%s: unknown parameter type: %d"), + funcname, param->type); goto cleanup; } } @@ -6987,19 +7001,12 @@ remoteDomainGetJobStats(virDomainPtr domain, (xdrproc_t) xdr_remote_domain_get_job_stats_ret, (char *) &ret) == -1) goto done; - if (ret.params.params_len > REMOTE_DOMAIN_JOB_STATS_MAX) { - virReportError(VIR_ERR_RPC, - _("Too many job stats '%d' for limit '%d'"), - ret.params.params_len, - REMOTE_DOMAIN_JOB_STATS_MAX); - goto cleanup; - } - *type = ret.type; if (remoteDeserializeTypedParameters(ret.params.params_val, ret.params.params_len, - 0, params, nparams) < 0) + REMOTE_DOMAIN_JOB_STATS_MAX, + params, nparams) < 0) goto cleanup; rv = 0; -- 1.9.3
On 09/13/14 15:27, John Ferlan wrote:
Since 98b9acf5aa02551dd37d0209339aba2e22e4004a
This was a false positive where Coverity was complaining that the remoteDeserializeTypedParameters() could allocate 'params', but none of the callers could return the allocated memory back to their caller since on input the param was passed by value. Additionally, the flow of the code was that if params was NULL on entry, then each function would return 'nparams' as the number of params entries the caller would need to allocate in order to call the function again with 'nparams' and 'params' being set. By the time the deserialize routine was called params would have something. For other callers where the 'params' was passed by reference as NULL since it's expected that the deserialize allocates the memory and then have that passed back to the original caller to dispose there was no Coverity issue.
As it turns out Coverity didn't quite seem to understand the relationship between 'nparams' and 'params'; however, if the !userAllocated path of the deserialize code compared against limit in any manner, then the Coverity error went away which was quite strange, but useful.
As it turns out one code path remoteDomainGetJobStats had a comparison against 'limit' while another remoteConnectGetAllDomainStats did not assuming that limit would be checked. So I refactored the code a bit to cause the limit check to occur in deserialize for both conditions and then only made the check of current returned size against the incoming *nparams fail the non allocation case. This means the job code doesn't need to check the limit any more, while the stats code now does check the limit.
Additionally, to help perhaps decipher which of the various callers to the deserialize code caused the failure - I used a #define to pass the __FUNCNAME__ of the caller along so that error messages could have something like:
error: remoteConnectGetAllDomainStats: too many parameters '2' for nparams '0' error: Reconnected to the hypervisor
(it's a contrived error just to show the funcname in the error)
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/remote/remote_driver.c | 49 ++++++++++++++++++++++++++-------------------- 1 file changed, 28 insertions(+), 21 deletions(-)
ACK, Peter
Coverity complains that because of how 'offset' is initialized to 0 (zero), the resulting math and comparison on rem is pointless. According to the origin commit id '3ec128989', the code is a replacement for gmtime(), but without the localtime() or GMT calculations - so just remove this code and add a comment indicating the removal Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virtime.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/src/util/virtime.c b/src/util/virtime.c index 9fefb67..acbec41 100644 --- a/src/util/virtime.c +++ b/src/util/virtime.c @@ -127,22 +127,16 @@ const unsigned short int __mon_yday[2][13] = { void virTimeFieldsThen(unsigned long long when, struct tm *fields) { /* This code is taken from GLibC under terms of LGPLv2+ */ + /* Remove the 'offset' or GMT manipulation since we don't care. See + * commit id '3ec12898' comments regarding localtime. + */ long int days, rem, y; const unsigned short int *ip; unsigned long long whenSecs = when / 1000ull; - unsigned int offset = 0; /* We hardcoded GMT */ days = whenSecs / SECS_PER_DAY; rem = whenSecs % SECS_PER_DAY; - rem += offset; - while (rem < 0) { - rem += SECS_PER_DAY; - --days; - } - while (rem >= SECS_PER_DAY) { - rem -= SECS_PER_DAY; - ++days; - } + fields->tm_hour = rem / SECS_PER_HOUR; rem %= SECS_PER_HOUR; fields->tm_min = rem / 60; -- 1.9.3
On 09/13/14 15:27, John Ferlan wrote:
Coverity complains that because of how 'offset' is initialized to 0 (zero), the resulting math and comparison on rem is pointless.
According to the origin commit id '3ec128989', the code is a replacement for gmtime(), but without the localtime() or GMT calculations - so just remove this code and add a comment indicating the removal
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virtime.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-)
ACK Peter
Coverity complains that the condition "size + 1 == 0" cannot happen. It's already been determined that offset+size is not larger than buf_size (and buf_size is smaller than UINT_MAX); and also that offset+size didn't overflow. So just remove the check Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virstoragefile.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 5db9184..1a02b18 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -393,8 +393,6 @@ qcowXGetBackingStore(char **res, } if (offset + size > buf_size || offset + size < offset) return BACKING_STORE_INVALID; - if (size + 1 == 0) - return BACKING_STORE_INVALID; if (VIR_ALLOC_N(*res, size + 1) < 0) return BACKING_STORE_ERROR; memcpy(*res, buf + offset, size); -- 1.9.3
On 09/13/14 15:27, John Ferlan wrote:
Coverity complains that the condition "size + 1 == 0" cannot happen. It's already been determined that offset+size is not larger than buf_size (and buf_size is smaller than UINT_MAX); and also that
buff_size smaller than UINT_MAX isn't guaranteed in this function. Although it will probably never happen the size is declared as size_t and thus equal size to unsigned long long on 64 bit machines.
offset+size didn't overflow.
So just remove the check
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virstoragefile.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 5db9184..1a02b18 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -393,8 +393,6 @@ qcowXGetBackingStore(char **res, } if (offset + size > buf_size || offset + size < offset)
If size is UINT_MAX, then when you add it to offset, which is a small number, then you get something between UINT_MAX and ULLONG_MAX which is still in range of buf_size as it's a size_t.
return BACKING_STORE_INVALID; - if (size + 1 == 0) - return BACKING_STORE_INVALID;
So you may have this condition theoretically true, while the check above doesn't catch it.
if (VIR_ALLOC_N(*res, size + 1) < 0) return BACKING_STORE_ERROR; memcpy(*res, buf + offset, size);
Also I've looked on the specs of the qcow2 header format and it seems that we don't do exactly the right thing here. To determine that the qcow2 image doesn't have a backing store we sholuld use the offset rather than size: 8 - 15: backing_file_offset Offset into the image file at which the backing file name is stored (NB: The string is not null terminated). 0 if the image doesn't have a backing file. 16 - 19: backing_file_size Length of the backing file name in bytes. Must not be longer than 1023 bytes. Undefined if the image doesn't have a backing file. Also we probably should make sure that the backing file size is less than 1024 bytes. While I agree that the check is dead code, as we call this function with buf_size with a reasonably low value, we probably should improve the backing file detection code to match the specs. Peter
On 09/15/2014 04:42 AM, Peter Krempa wrote:
On 09/13/14 15:27, John Ferlan wrote:
Coverity complains that the condition "size + 1 == 0" cannot happen. It's already been determined that offset+size is not larger than buf_size (and buf_size is smaller than UINT_MAX); and also that
buff_size smaller than UINT_MAX isn't guaranteed in this function. Although it will probably never happen the size is declared as size_t and thus equal size to unsigned long long on 64 bit machines.
offset+size didn't overflow.
So just remove the check
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virstoragefile.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 5db9184..1a02b18 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -393,8 +393,6 @@ qcowXGetBackingStore(char **res, } if (offset + size > buf_size || offset + size < offset)
If size is UINT_MAX, then when you add it to offset, which is a small number, then you get something between UINT_MAX and ULLONG_MAX which is still in range of buf_size as it's a size_t.
return BACKING_STORE_INVALID; - if (size + 1 == 0) - return BACKING_STORE_INVALID;
So you may have this condition theoretically true, while the check above doesn't catch it.
My previous change was : + if (size == UINT_MAX) return BACKING_STORE_INVALID; But it was pointed out by eblake: " Is this dead code? After all, we just checked that offset+size is not larger than buf_size (and buf_size is smaller than UINT_MAX); and also that offset+size didn't overflow. " Would it be better to use the original change instead? Just trying to get past Coverity issue on this... BTW: The remainder of this w/r/t matching spec seems to be outside the scope of the Coverity DEADCODE, but if you have a patch I'm more than willing to look at it ;-) This code is shared with qcow1GetBackingStore and it's not like it's a recent change, so I'm hesitant to change it... John
if (VIR_ALLOC_N(*res, size + 1) < 0) return BACKING_STORE_ERROR; memcpy(*res, buf + offset, size);
Also I've looked on the specs of the qcow2 header format and it seems that we don't do exactly the right thing here. To determine that the qcow2 image doesn't have a backing store we sholuld use the offset rather than size:
8 - 15: backing_file_offset Offset into the image file at which the backing file name is stored (NB: The string is not null terminated). 0 if the image doesn't have a backing file.
16 - 19: backing_file_size Length of the backing file name in bytes. Must not be longer than 1023 bytes. Undefined if the image doesn't have a backing file.
Also we probably should make sure that the backing file size is less than 1024 bytes.
While I agree that the check is dead code, as we call this function with buf_size with a reasonably low value, we probably should improve the backing file detection code to match the specs.
Peter
On 09/15/14 15:23, John Ferlan wrote:
On 09/15/2014 04:42 AM, Peter Krempa wrote:
On 09/13/14 15:27, John Ferlan wrote:
Coverity complains that the condition "size + 1 == 0" cannot happen. It's already been determined that offset+size is not larger than buf_size (and buf_size is smaller than UINT_MAX); and also that
buff_size smaller than UINT_MAX isn't guaranteed in this function. Although it will probably never happen the size is declared as size_t and thus equal size to unsigned long long on 64 bit machines.
offset+size didn't overflow.
So just remove the check
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virstoragefile.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 5db9184..1a02b18 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -393,8 +393,6 @@ qcowXGetBackingStore(char **res, } if (offset + size > buf_size || offset + size < offset)
If size is UINT_MAX, then when you add it to offset, which is a small number, then you get something between UINT_MAX and ULLONG_MAX which is still in range of buf_size as it's a size_t.
return BACKING_STORE_INVALID; - if (size + 1 == 0) - return BACKING_STORE_INVALID;
So you may have this condition theoretically true, while the check above doesn't catch it.
My previous change was :
+ if (size == UINT_MAX) return BACKING_STORE_INVALID;
But it was pointed out by eblake:
" Is this dead code? After all, we just checked that offset+size is not larger than buf_size (and buf_size is smaller than UINT_MAX); and also
I wasn't able to locate the part that guarantees that buf_size is < UINT_MAX.
that offset+size didn't overflow.
"
Would it be better to use the original change instead? Just trying to get past Coverity issue on this...
BTW: The remainder of this w/r/t matching spec seems to be outside the scope of the Coverity DEADCODE, but if you have a patch I'm more than willing to look at it ;-) This code is shared with qcow1GetBackingStore and it's not like it's a recent change, so I'm hesitant to change it...
I've sent a patch that should fix the parser according to the docs and also should fix the dead code here.
John
Peter
Seems when commit id 'ea130e3b' added the checks to ensure each of the hard_limit, soft_limit, and swap_hard_limit wasn't set at VIR_DOMAIN_MEMORY_PARAM_UNLIMITED - a copy/paste error of using the 'hard_limit' for each comparison was done. Adjust the code. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a2a7d92..44db5d8 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -18088,9 +18088,9 @@ virDomainDefFormatInternal(virDomainDefPtr def, if ((def->mem.hard_limit && def->mem.hard_limit != VIR_DOMAIN_MEMORY_PARAM_UNLIMITED) || (def->mem.soft_limit && - def->mem.hard_limit != VIR_DOMAIN_MEMORY_PARAM_UNLIMITED) || + def->mem.soft_limit != VIR_DOMAIN_MEMORY_PARAM_UNLIMITED) || (def->mem.swap_hard_limit && - def->mem.hard_limit != VIR_DOMAIN_MEMORY_PARAM_UNLIMITED) || + def->mem.swap_hard_limit != VIR_DOMAIN_MEMORY_PARAM_UNLIMITED) || def->mem.min_guarantee) { virBufferAddLit(buf, "<memtune>\n"); virBufferAdjustIndent(buf, 2); -- 1.9.3
On 09/13/14 15:27, John Ferlan wrote:
Seems when commit id 'ea130e3b' added the checks to ensure each of the hard_limit, soft_limit, and swap_hard_limit wasn't set at VIR_DOMAIN_MEMORY_PARAM_UNLIMITED - a copy/paste error of using the 'hard_limit' for each comparison was done. Adjust the code.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
ACK, introduced in commit ea130e3b Peter
Coverity complains that on the first pass through the for loop that 'params' cannot be true, thus the ternary setting to "&" cannot be done - which is I believe true - thus this really is a deadcode situation. It doesn't complain on the following comparison for "spice" - of course because we've set params = true now. I believe that's correct since we're putting together the line and the first "param" sets the "?" while the rest use "&". Signed-off-by: John Ferlan <jferlan@redhat.com> --- tools/virsh-domain.c | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index f80741c..a040e27 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -9824,6 +9824,7 @@ cmdDomDisplay(vshControl *ctl, const vshCmd *cmd) /* TLS Port */ if (tls_port) { + /* coverity[dead_error_condition] */ virBufferAsprintf(&buf, "%stls-port=%d", params ? "&" : "?", -- 1.9.3
On 09/13/14 15:27, John Ferlan wrote:
Coverity complains that on the first pass through the for loop that 'params' cannot be true, thus the ternary setting to "&" cannot be done - which is I believe true - thus this really is a deadcode situation.
It doesn't complain on the following comparison for "spice" - of course because we've set params = true now. I believe that's correct since we're putting together the line and the first "param" sets the "?" while the rest use "&".
Signed-off-by: John Ferlan <jferlan@redhat.com> --- tools/virsh-domain.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index f80741c..a040e27 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -9824,6 +9824,7 @@ cmdDomDisplay(vshControl *ctl, const vshCmd *cmd)
/* TLS Port */ if (tls_port) { + /* coverity[dead_error_condition] */ virBufferAsprintf(&buf, "%stls-port=%d", params ? "&" : "?",
Since the for loop reaches the point at most once, this is really doesn't have the chance to be executed with params equal to true. This was probably done to have it consistent.
We might as well drop the params and hardcode the "?". ACK to both approaches. Peter
Coverity complained that checking the return of virDomainCreate() was not consistent amongst the callers - so added the return check to the objecteventtest.c and adjust the virt-login-shell to compare < 0 rather than just non zero for the failure condition. Signed-off-by: John Ferlan <jferlan@redhat.com> --- tests/objecteventtest.c | 3 ++- tools/virt-login-shell.c | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/objecteventtest.c b/tests/objecteventtest.c index 919f559..052dbe5 100644 --- a/tests/objecteventtest.c +++ b/tests/objecteventtest.c @@ -359,7 +359,8 @@ testDomainStartStopEvent(const void *data) /* Test domain is started */ virDomainDestroy(dom); - virDomainCreate(dom); + if (virDomainCreate(dom) < 0) + goto cleanup; if (virEventRunDefaultImpl() < 0) goto cleanup; diff --git a/tools/virt-login-shell.c b/tools/virt-login-shell.c index be15a32..ceb271d 100644 --- a/tools/virt-login-shell.c +++ b/tools/virt-login-shell.c @@ -275,7 +275,7 @@ main(int argc, char **argv) if (!dom) goto cleanup; - if (!virDomainIsActive(dom) && virDomainCreate(dom)) { + if (!virDomainIsActive(dom) && virDomainCreate(dom) < 0) { virErrorPtr last_error; last_error = virGetLastError(); if (last_error->code != VIR_ERR_OPERATION_INVALID) { -- 1.9.3
On 09/13/14 15:27, John Ferlan wrote:
Coverity complained that checking the return of virDomainCreate() was not consistent amongst the callers - so added the return check to the objecteventtest.c and adjust the virt-login-shell to compare < 0 rather than just non zero for the failure condition.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- tests/objecteventtest.c | 3 ++- tools/virt-login-shell.c | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/tests/objecteventtest.c b/tests/objecteventtest.c index 919f559..052dbe5 100644 --- a/tests/objecteventtest.c +++ b/tests/objecteventtest.c @@ -359,7 +359,8 @@ testDomainStartStopEvent(const void *data)
/* Test domain is started */ virDomainDestroy(dom); - virDomainCreate(dom); + if (virDomainCreate(dom) < 0) + goto cleanup;
if (virEventRunDefaultImpl() < 0) goto cleanup; diff --git a/tools/virt-login-shell.c b/tools/virt-login-shell.c index be15a32..ceb271d 100644 --- a/tools/virt-login-shell.c +++ b/tools/virt-login-shell.c @@ -275,7 +275,7 @@ main(int argc, char **argv) if (!dom) goto cleanup;
- if (!virDomainIsActive(dom) && virDomainCreate(dom)) { + if (!virDomainIsActive(dom) && virDomainCreate(dom) < 0) {
This shouldn't be necessary, it returns only 0 or -1 although if it shuts coverity up then it's fine with me.
virErrorPtr last_error; last_error = virGetLastError(); if (last_error->code != VIR_ERR_OPERATION_INVALID) {
ACK, Peter
Coverity complains about the calculation of the buf & len within the PROBE macro. So to quiet things down, do the calculation prior to usage in either write() or qemuMonitorIOWriteWithFD() calls and then have the PROBE use the calculated values - which works. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_monitor.c | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 6059133..80c6ef8 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -478,6 +478,8 @@ static int qemuMonitorIOWrite(qemuMonitorPtr mon) { int done; + char *buf; + int len; /* If no active message, or fully transmitted, the no-op */ if (!mon->msg || mon->msg->txOffset == mon->msg->txLength) @@ -489,22 +491,16 @@ qemuMonitorIOWrite(qemuMonitorPtr mon) return -1; } + buf = mon->msg->txBuffer + mon->msg->txOffset; + len = mon->msg->txLength - mon->msg->txOffset; if (mon->msg->txFD == -1) - done = write(mon->fd, - mon->msg->txBuffer + mon->msg->txOffset, - mon->msg->txLength - mon->msg->txOffset); + done = write(mon->fd, buf, len); else - done = qemuMonitorIOWriteWithFD(mon, - mon->msg->txBuffer + mon->msg->txOffset, - mon->msg->txLength - mon->msg->txOffset, - mon->msg->txFD); + done = qemuMonitorIOWriteWithFD(mon, buf, len, mon->msg->txFD); PROBE(QEMU_MONITOR_IO_WRITE, "mon=%p buf=%s len=%d ret=%d errno=%d", - mon, - mon->msg->txBuffer + mon->msg->txOffset, - mon->msg->txLength - mon->msg->txOffset, - done, errno); + mon, buf, len, done, errno); if (mon->msg->txFD != -1) { PROBE(QEMU_MONITOR_IO_SEND_FD, -- 1.9.3
On 09/13/14 15:27, John Ferlan wrote:
Coverity complains about the calculation of the buf & len within the PROBE macro. So to quiet things down, do the calculation prior to usage in either write() or qemuMonitorIOWriteWithFD() calls and then have the PROBE use the calculated values - which works.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_monitor.c | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 6059133..80c6ef8 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -478,6 +478,8 @@ static int qemuMonitorIOWrite(qemuMonitorPtr mon) { int done; + char *buf; + int len;
len should be "size_t"
/* If no active message, or fully transmitted, the no-op */ if (!mon->msg || mon->msg->txOffset == mon->msg->txLength) @@ -489,22 +491,16 @@ qemuMonitorIOWrite(qemuMonitorPtr mon) return -1; }
+ buf = mon->msg->txBuffer + mon->msg->txOffset; + len = mon->msg->txLength - mon->msg->txOffset; if (mon->msg->txFD == -1) - done = write(mon->fd, - mon->msg->txBuffer + mon->msg->txOffset, - mon->msg->txLength - mon->msg->txOffset); + done = write(mon->fd, buf, len); else - done = qemuMonitorIOWriteWithFD(mon, - mon->msg->txBuffer + mon->msg->txOffset, - mon->msg->txLength - mon->msg->txOffset, - mon->msg->txFD); + done = qemuMonitorIOWriteWithFD(mon, buf, len, mon->msg->txFD);
PROBE(QEMU_MONITOR_IO_WRITE, "mon=%p buf=%s len=%d ret=%d errno=%d", - mon, - mon->msg->txBuffer + mon->msg->txOffset, - mon->msg->txLength - mon->msg->txOffset, - done, errno); + mon, buf, len, done, errno);
if (mon->msg->txFD != -1) { PROBE(QEMU_MONITOR_IO_SEND_FD,
ACK with the type corrected. Peter
On 09/15/2014 04:09 AM, Peter Krempa wrote:
On 09/13/14 15:27, John Ferlan wrote:
Coverity complains about the calculation of the buf & len within the PROBE macro. So to quiet things down, do the calculation prior to usage in either write() or qemuMonitorIOWriteWithFD() calls and then have the PROBE use the calculated values - which works.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_monitor.c | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 6059133..80c6ef8 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -478,6 +478,8 @@ static int qemuMonitorIOWrite(qemuMonitorPtr mon) { int done; + char *buf; + int len;
len should be "size_t"
Of course seen when formatting %d on the PROBE() I was reminded I had to change the type of the printf format ;-)... Also, I would be remiss if I didn't point out... struct _qemuMonitorMessage { int txFD; char *txBuffer; int txOffset; int txLength; ... John
/* If no active message, or fully transmitted, the no-op */ if (!mon->msg || mon->msg->txOffset == mon->msg->txLength) @@ -489,22 +491,16 @@ qemuMonitorIOWrite(qemuMonitorPtr mon) return -1; }
+ buf = mon->msg->txBuffer + mon->msg->txOffset; + len = mon->msg->txLength - mon->msg->txOffset; if (mon->msg->txFD == -1) - done = write(mon->fd, - mon->msg->txBuffer + mon->msg->txOffset, - mon->msg->txLength - mon->msg->txOffset); + done = write(mon->fd, buf, len); else - done = qemuMonitorIOWriteWithFD(mon, - mon->msg->txBuffer + mon->msg->txOffset, - mon->msg->txLength - mon->msg->txOffset, - mon->msg->txFD); + done = qemuMonitorIOWriteWithFD(mon, buf, len, mon->msg->txFD);
PROBE(QEMU_MONITOR_IO_WRITE, "mon=%p buf=%s len=%d ret=%d errno=%d", - mon, - mon->msg->txBuffer + mon->msg->txOffset, - mon->msg->txLength - mon->msg->txOffset, - done, errno); + mon, buf, len, done, errno);
if (mon->msg->txFD != -1) { PROBE(QEMU_MONITOR_IO_SEND_FD,
ACK with the type corrected.
Peter
On 09/15/14 16:57, John Ferlan wrote:
On 09/15/2014 04:09 AM, Peter Krempa wrote:
On 09/13/14 15:27, John Ferlan wrote:
Coverity complains about the calculation of the buf & len within the PROBE macro. So to quiet things down, do the calculation prior to usage in either write() or qemuMonitorIOWriteWithFD() calls and then have the PROBE use the calculated values - which works.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_monitor.c | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 6059133..80c6ef8 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -478,6 +478,8 @@ static int qemuMonitorIOWrite(qemuMonitorPtr mon) { int done; + char *buf; + int len;
len should be "size_t"
Of course seen when formatting %d on the PROBE() I was reminded I had to change the type of the printf format ;-)... Also, I would be remiss if I didn't point out...
struct _qemuMonitorMessage { int txFD;
char *txBuffer; int txOffset; int txLength;
Sigh, definitely not the last place with incorrect types :)
On 09/15/2014 04:09 AM, Peter Krempa wrote:
On 09/13/14 15:27, John Ferlan wrote:
Coverity complains about the calculation of the buf & len within the PROBE macro. So to quiet things down, do the calculation prior to usage in either write() or qemuMonitorIOWriteWithFD() calls and then have the PROBE use the calculated values - which works.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_monitor.c | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-)
A build failure on a local jenkins server resulted in the following being pushed : commit c29cad67328bb8845c4396b85884211124ea1e2d Author: John Ferlan <jferlan@redhat.com> Date: Mon Sep 15 11:37:20 2014 -0400 qemu: Fix build breaker on printf directive %zu for size_t not %lu diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 89446d7..3a32a4f 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -499,7 +499,7 @@ qemuMonitorIOWrite(qemuMonitorPtr mon) done = qemuMonitorIOWriteWithFD(mon, buf, len, mon->msg->txFD); PROBE(QEMU_MONITOR_IO_WRITE, - "mon=%p buf=%s len=%lu ret=%d errno=%d", + "mon=%p buf=%s len=%zu ret=%d errno=%d", mon, buf, len, done, errno); if (mon->msg->txFD != -1) {
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 6059133..80c6ef8 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -478,6 +478,8 @@ static int qemuMonitorIOWrite(qemuMonitorPtr mon) { int done; + char *buf; + int len;
len should be "size_t"
/* If no active message, or fully transmitted, the no-op */ if (!mon->msg || mon->msg->txOffset == mon->msg->txLength) @@ -489,22 +491,16 @@ qemuMonitorIOWrite(qemuMonitorPtr mon) return -1; }
+ buf = mon->msg->txBuffer + mon->msg->txOffset; + len = mon->msg->txLength - mon->msg->txOffset; if (mon->msg->txFD == -1) - done = write(mon->fd, - mon->msg->txBuffer + mon->msg->txOffset, - mon->msg->txLength - mon->msg->txOffset); + done = write(mon->fd, buf, len); else - done = qemuMonitorIOWriteWithFD(mon, - mon->msg->txBuffer + mon->msg->txOffset, - mon->msg->txLength - mon->msg->txOffset, - mon->msg->txFD); + done = qemuMonitorIOWriteWithFD(mon, buf, len, mon->msg->txFD);
PROBE(QEMU_MONITOR_IO_WRITE, "mon=%p buf=%s len=%d ret=%d errno=%d", - mon, - mon->msg->txBuffer + mon->msg->txOffset, - mon->msg->txLength - mon->msg->txOffset, - done, errno); + mon, buf, len, done, errno);
if (mon->msg->txFD != -1) { PROBE(QEMU_MONITOR_IO_SEND_FD,
ACK with the type corrected.
Peter
Coverity complains that the comparison: if (nfds && nfds > ((int)!!sock_path + (int)!!sock_path_ro)) could mean 'sock_path' is NULL. Later in virNetSocketNewListenUNIX there's a direct dereference of path in the error path: if (path[0] != '@') A bit of sleuthing proves that upon entry to daemonSetupNetworking there is no way for 'sock_path' to be NULL since daemonUnixSocketPaths will set up 'sock_file' (although it may not set up 'sock_file_ro') in all 3 paths. So to keep Coverity quiet - add an sa_assert prior to the call. Signed-off-by: John Ferlan <jferlan@redhat.com> --- daemon/libvirtd.c | 1 + 1 file changed, 1 insertion(+) diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 05ee50e..028145e 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -502,6 +502,7 @@ static int daemonSetupNetworking(virNetServerPtr srv, return -1; } + sa_assert(sock_path); if (nfds && nfds > ((int)!!sock_path + (int)!!sock_path_ro)) { VIR_ERROR(_("Too many (%u) FDs passed from caller"), nfds); return -1; -- 1.9.3
On Sat, Sep 13, 2014 at 09:27:45AM -0400, John Ferlan wrote:
Coverity complains that the comparison:
if (nfds && nfds > ((int)!!sock_path + (int)!!sock_path_ro))
could mean 'sock_path' is NULL. Later in virNetSocketNewListenUNIX there's a direct dereference of path in the error path:
if (path[0] != '@')
A bit of sleuthing proves that upon entry to daemonSetupNetworking there is no way for 'sock_path' to be NULL since daemonUnixSocketPaths will set up 'sock_file' (although it may not set up 'sock_file_ro') in all 3 paths.
So to keep Coverity quiet - add an sa_assert prior to the call.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- daemon/libvirtd.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 05ee50e..028145e 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -502,6 +502,7 @@ static int daemonSetupNetworking(virNetServerPtr srv, return -1; }
+ sa_assert(sock_path); if (nfds && nfds > ((int)!!sock_path + (int)!!sock_path_ro)) {
Well, if sock_path cannot be NULL, then why not change it to: if (nfds > (1 + !!sock_path_ro)) { or something like that? Martin
VIR_ERROR(_("Too many (%u) FDs passed from caller"), nfds); return -1; -- 1.9.3
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 09/15/2014 04:36 AM, Martin Kletzander wrote:
On Sat, Sep 13, 2014 at 09:27:45AM -0400, John Ferlan wrote:
Coverity complains that the comparison:
if (nfds && nfds > ((int)!!sock_path + (int)!!sock_path_ro))
could mean 'sock_path' is NULL. Later in virNetSocketNewListenUNIX there's a direct dereference of path in the error path:
if (path[0] != '@')
A bit of sleuthing proves that upon entry to daemonSetupNetworking there is no way for 'sock_path' to be NULL since daemonUnixSocketPaths will set up 'sock_file' (although it may not set up 'sock_file_ro') in all 3 paths.
So to keep Coverity quiet - add an sa_assert prior to the call.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- daemon/libvirtd.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 05ee50e..028145e 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -502,6 +502,7 @@ static int daemonSetupNetworking(virNetServerPtr srv, return -1; }
+ sa_assert(sock_path); if (nfds && nfds > ((int)!!sock_path + (int)!!sock_path_ro)) {
Well, if sock_path cannot be NULL, then why not change it to:
if (nfds > (1 + !!sock_path_ro)) {
or something like that?
Sure, but while I ensured that "today" - who's to say tomorrow someone doesn't make a change in the future that alters that assumption? Just removing "sock_path" still "hides" the fact that someone could incorrectly pass a NULL sock_path here. At least the sa_assert() will cover that. The "real" reason Coverity complains is because sock_path is eventually dereferenced elsewhere without first checking if it's NULL, but Coverity believes this code is check for it being NULL. It's a double edged sword. Another "option" would be to adjust the code in that place to ensure path is valid before dereferencing. Of course, knowing that there is another change pending in this area that I reviewed and that it also makes the same deref, I think not using sa_assert here would be the gift that keeps on giving with respect to Coverity complaints. So, how about the following instead (for equally challenging ternary operations): $ git diff diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 893aacf..5b8f28e 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -442,12 +442,13 @@ static void daemonInitialize(void) } -static int daemonSetupNetworking(virNetServerPtr srv, - struct daemonConfig *config, - const char *sock_path, - const char *sock_path_ro, - bool ipsock, - bool privileged) +static int ATTRIBUTE_NONNULL(3) +daemonSetupNetworking(virNetServerPtr srv, + struct daemonConfig *config, + const char *sock_path, + const char *sock_path_ro, + bool ipsock, + bool privileged) { virNetServerServicePtr svc = NULL; virNetServerServicePtr svcRO = NULL; @@ -467,8 +468,7 @@ static int daemonSetupNetworking(virNetServerPtr srv, return -1; } - sa_assert(sock_path); - if (nfds && nfds > ((int)!!sock_path + (int)!!sock_path_ro)) { + if (nfds && nfds > (sock_path_ro ? 2 : 1)) { VIR_ERROR(_("Too many (%u) FDs passed from caller"), nfds); return -1; } John
Martin
VIR_ERROR(_("Too many (%u) FDs passed from caller"), nfds); return -1; -- 1.9.3
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Sep 15, 2014 at 07:49:09AM -0400, John Ferlan wrote:
On 09/15/2014 04:36 AM, Martin Kletzander wrote:
On Sat, Sep 13, 2014 at 09:27:45AM -0400, John Ferlan wrote:
Coverity complains that the comparison:
if (nfds && nfds > ((int)!!sock_path + (int)!!sock_path_ro))
could mean 'sock_path' is NULL. Later in virNetSocketNewListenUNIX there's a direct dereference of path in the error path:
if (path[0] != '@')
A bit of sleuthing proves that upon entry to daemonSetupNetworking there is no way for 'sock_path' to be NULL since daemonUnixSocketPaths will set up 'sock_file' (although it may not set up 'sock_file_ro') in all 3 paths.
So to keep Coverity quiet - add an sa_assert prior to the call.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- daemon/libvirtd.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 05ee50e..028145e 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -502,6 +502,7 @@ static int daemonSetupNetworking(virNetServerPtr srv, return -1; }
+ sa_assert(sock_path); if (nfds && nfds > ((int)!!sock_path + (int)!!sock_path_ro)) {
Well, if sock_path cannot be NULL, then why not change it to:
if (nfds > (1 + !!sock_path_ro)) {
or something like that?
Sure, but while I ensured that "today" - who's to say tomorrow someone doesn't make a change in the future that alters that assumption? Just removing "sock_path" still "hides" the fact that someone could incorrectly pass a NULL sock_path here. At least the sa_assert() will cover that.
The "real" reason Coverity complains is because sock_path is eventually dereferenced elsewhere without first checking if it's NULL, but Coverity believes this code is check for it being NULL. It's a double edged sword.
Well, then that sa_assert() would make sense being there (where the dereference happens) or having a check only for that in some meaningful place. But I believe we're trying to solve a completely different problem. I was just stupid when I wrote the code :)
Another "option" would be to adjust the code in that place to ensure path is valid before dereferencing. Of course, knowing that there is another change pending in this area that I reviewed and that it also makes the same deref, I think not using sa_assert here would be the gift that keeps on giving with respect to Coverity complaints.
But having it where you proposed it doesn't really make sense to me (or is not visible at first).
So, how about the following instead (for equally challenging ternary operations):
$ git diff diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 893aacf..5b8f28e 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -442,12 +442,13 @@ static void daemonInitialize(void) }
-static int daemonSetupNetworking(virNetServerPtr srv, - struct daemonConfig *config, - const char *sock_path, - const char *sock_path_ro, - bool ipsock, - bool privileged) +static int ATTRIBUTE_NONNULL(3) +daemonSetupNetworking(virNetServerPtr srv, + struct daemonConfig *config, + const char *sock_path, + const char *sock_path_ro, + bool ipsock, + bool privileged) { virNetServerServicePtr svc = NULL; virNetServerServicePtr svcRO = NULL; @@ -467,8 +468,7 @@ static int daemonSetupNetworking(virNetServerPtr srv, return -1; }
- sa_assert(sock_path); - if (nfds && nfds > ((int)!!sock_path + (int)!!sock_path_ro)) { + if (nfds && nfds > (sock_path_ro ? 2 : 1)) { VIR_ERROR(_("Too many (%u) FDs passed from caller"), nfds); return -1; }
Yes, and you can remove the "nfds &&" I believe (I have no idea why I used it there). ACK with this squashed in. Martin
On 09/13/2014 09:27 AM, John Ferlan wrote:
After this pile of changes I am back down to zero issues.
Patch 1 - is a repeat offender - this is the 3rd try, but I think I have a better solution here. Details are in the commit message.
Patch 2-3 - are repeats from the last series as patches 6 and 9. For the virtime.c code - I just removed the offending math and deadcode leaving the entrails in the commit message and code. For the virstoragefile.c code - I agree with eblake's logic and just removed the entire check as dead.
Patch 4 - seems to be a real issue. Initially I thought it wasn't, but after finding the original commit it seems it was a copypaste type error. I initially thought perhaps if hard_limit was set as the unlimited value that the design was don't change soft_limit and swap_hard_limit
Patch 5 - false positive
Patch 6 - false positive mostly, but just add the proper checks like other code
Patch 7 - the BAD_SIZEOF was being triggered on the math to the PROBE macro. By generating local variables - the issue went away
Patch 8 - initially I wasn't so sure on this - that !!sock_path is just one of those constructs I find "odd" to read in code. Anyway, as it turns out this is a false positive as there is no way sock_path could be null here - so I just added the sa_assert() and that satisfies Coverity
John Ferlan (8): remote_driver: Resolve Coverity RESOURCE_LEAK virtime: Resolve Coverity DEADCODE virstoragefile: Resolve Coverity DEADCODE domain_conf: Resolve Coverity COPY_PASTE_ERROR virsh: Resolve Coverity DEADCODE Resolve Coverity CHECKED_RETURN qemu: Resolve Coverity BAD_SIZEOF daemon: Resolve Coverity FORWARD_NULL
daemon/libvirtd.c | 1 + src/conf/domain_conf.c | 4 ++-- src/qemu/qemu_monitor.c | 18 +++++++---------- src/remote/remote_driver.c | 49 ++++++++++++++++++++++++++-------------------- src/util/virstoragefile.c | 2 -- src/util/virtime.c | 14 ++++--------- tests/objecteventtest.c | 3 ++- tools/virsh-domain.c | 1 + tools/virt-login-shell.c | 2 +- 9 files changed, 46 insertions(+), 48 deletions(-)
Other than patch 3 which is taken care of by : http://www.redhat.com/archives/libvir-list/2014-September/msg00910.html These are now pushed with the nits/issues fixed of course. Tks, John
participants (3)
-
John Ferlan -
Martin Kletzander -
Peter Krempa