[libvirt] [PATCH 0/6] qemu: util: vz: some fixes and improvements
Maxim Nestratov (5): vz: get additional error information from job correctly util: fix crash in virClassIsDerivedFrom for CloseCallbacks objects vz: don't fail query domain info in case we don't have valid stats handle vz: reset errors after ignoring return values vz: specify VIR_DOMAIN_NET_TYPE_NETWORK for routed networks Yuri Pudgorodskiy (1): qemu: guest agent: introduce new error code VIR_ERR_AGENT_UNSYNCED include/libvirt/virterror.h | 2 ++ src/qemu/qemu_agent.c | 6 +++--- src/util/virclosecallbacks.c | 3 +++ src/util/virerror.c | 6 ++++++ src/vz/vz_driver.c | 13 +++++++++---- src/vz/vz_sdk.c | 41 +++++++++++++++++++++++------------------ 6 files changed, 46 insertions(+), 25 deletions(-) -- 1.8.3.1
From: Yuri Pudgorodskiy <yur@virtuozzo.com> A separate error code will help recognize real failures from necessity to try again Signed-off-by: Maxim Nestratov <mnestratov@virtuozzo.com> --- include/libvirt/virterror.h | 2 ++ src/qemu/qemu_agent.c | 6 +++--- src/util/virerror.c | 6 ++++++ 3 files changed, 11 insertions(+), 3 deletions(-) diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h index 2ec560e..efe83aa 100644 --- a/include/libvirt/virterror.h +++ b/include/libvirt/virterror.h @@ -315,6 +315,8 @@ typedef enum { VIR_ERR_AUTH_UNAVAILABLE = 94, /* authentication unavailable */ VIR_ERR_NO_SERVER = 95, /* Server was not found */ VIR_ERR_NO_CLIENT = 96, /* Client was not found */ + VIR_ERR_AGENT_UNSYNCED = 97, /* guest agent replies with wrong id + to guest-sync command */ } virErrorNumber; /** diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index 0c1cf1c..20b724e 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -966,21 +966,21 @@ qemuAgentGuestSync(qemuAgentPtr mon) goto cleanup; if (!sync_msg.rxObject) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + virReportError(VIR_ERR_AGENT_UNSYNCED, "%s", _("Missing monitor reply object")); goto cleanup; } if (virJSONValueObjectGetNumberUlong(sync_msg.rxObject, "return", &id_ret) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + virReportError(VIR_ERR_AGENT_UNSYNCED, "%s", _("Malformed return value")); goto cleanup; } VIR_DEBUG("Guest returned ID: %llu", id_ret); if (id_ret != id) { - virReportError(VIR_ERR_INTERNAL_ERROR, + virReportError(VIR_ERR_AGENT_UNSYNCED, _("Guest agent returned ID: %llu instead of %llu"), id_ret, id); goto cleanup; diff --git a/src/util/virerror.c b/src/util/virerror.c index 1177570..2958308 100644 --- a/src/util/virerror.c +++ b/src/util/virerror.c @@ -1394,6 +1394,12 @@ virErrorMsg(virErrorNumber error, const char *info) else errmsg = _("Client not found: %s"); break; + case VIR_ERR_AGENT_UNSYNCED: + if (info == NULL) + errmsg = _("guest agent replied with wrong id to guest-sync command"); + else + errmsg = _("guest agent replied with wrong id to guest-sync command: %s"); + break; } return errmsg; } -- 1.8.3.1
First, make function logPrlEventErrorHelper be void and only print information (if any) from an event. Second, don't rewrite original error with any errors we get during parsing event info. Third, ignore PRL_ERR_NO_DATA at all. Signed-off-by: Maxim Nestratov <mnestratov@virtuozzo.com> --- src/vz/vz_sdk.c | 32 ++++++++++++++------------------ 1 file changed, 14 insertions(+), 18 deletions(-) diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index f81b320..45cac65 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -105,19 +105,12 @@ logPrlErrorHelper(PRL_RESULT err, const char *filename, } \ } while (0) -static PRL_RESULT +static void logPrlEventErrorHelper(PRL_HANDLE event, const char *filename, const char *funcname, size_t linenr) { - PRL_RESULT ret, retCode; char *msg1 = NULL, *msg2 = NULL; PRL_UINT32 len = 0; - int err = -1; - - if ((ret = PrlEvent_GetErrCode(event, &retCode))) { - logPrlError(ret); - return ret; - } PrlEvent_GetErrString(event, PRL_TRUE, PRL_FALSE, NULL, &len); @@ -136,13 +129,9 @@ logPrlEventErrorHelper(PRL_HANDLE event, const char *filename, virReportErrorHelper(VIR_FROM_THIS, VIR_ERR_INTERNAL_ERROR, filename, funcname, linenr, _("%s %s"), msg1, msg2); - err = 0; - cleanup: VIR_FREE(msg1); VIR_FREE(msg2); - - return err; } static PRL_RESULT @@ -152,12 +141,12 @@ getJobResultHelper(PRL_HANDLE job, unsigned int timeout, PRL_HANDLE *result, { PRL_RESULT ret, retCode; - if ((ret = PrlJob_Wait(job, timeout))) { + if (PRL_FAILED(ret = PrlJob_Wait(job, timeout))) { logPrlErrorHelper(ret, filename, funcname, linenr); goto cleanup; } - if ((ret = PrlJob_GetRetCode(job, &retCode))) { + if (PRL_FAILED(ret = PrlJob_GetRetCode(job, &retCode))) { logPrlErrorHelper(ret, filename, funcname, linenr); goto cleanup; } @@ -165,17 +154,24 @@ getJobResultHelper(PRL_HANDLE job, unsigned int timeout, PRL_HANDLE *result, if (retCode) { PRL_HANDLE err_handle; + ret = retCode; + /* Sometimes it's possible to get additional error info. */ - if ((ret = PrlJob_GetError(job, &err_handle))) { + if (PRL_FAILED(ret = PrlJob_GetError(job, &err_handle))) { logPrlErrorHelper(ret, filename, funcname, linenr); goto cleanup; } - if (logPrlEventErrorHelper(err_handle, filename, funcname, linenr)) - logPrlErrorHelper(retCode, filename, funcname, linenr); + if (PRL_FAILED(retCode = PrlEvent_GetErrCode(err_handle, &retCode))) { + logPrlErrorHelper(ret, filename, funcname, linenr); + if (PRL_ERR_NO_DATA != retCode) + logPrlError(retCode); + goto cleanup; + } + + logPrlEventErrorHelper(err_handle, filename, funcname, linenr); PrlHandle_Free(err_handle); - ret = retCode; } else { ret = PrlJob_GetResult(job, result); if (PRL_FAILED(ret)) { -- 1.8.3.1
On 10.08.2016 20:44, Maxim Nestratov wrote:
First, make function logPrlEventErrorHelper be void and only print information (if any) from an event. Second, don't rewrite original error with any errors we get during parsing event info. Third, ignore PRL_ERR_NO_DATA at all.
Signed-off-by: Maxim Nestratov <mnestratov@virtuozzo.com> --- src/vz/vz_sdk.c | 32 ++++++++++++++------------------ 1 file changed, 14 insertions(+), 18 deletions(-)
diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index f81b320..45cac65 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -105,19 +105,12 @@ logPrlErrorHelper(PRL_RESULT err, const char *filename, } \ } while (0)
-static PRL_RESULT +static void logPrlEventErrorHelper(PRL_HANDLE event, const char *filename, const char *funcname, size_t linenr) { - PRL_RESULT ret, retCode; char *msg1 = NULL, *msg2 = NULL; PRL_UINT32 len = 0; - int err = -1; - - if ((ret = PrlEvent_GetErrCode(event, &retCode))) { - logPrlError(ret); - return ret; - }
PrlEvent_GetErrString(event, PRL_TRUE, PRL_FALSE, NULL, &len);
@@ -136,13 +129,9 @@ logPrlEventErrorHelper(PRL_HANDLE event, const char *filename, virReportErrorHelper(VIR_FROM_THIS, VIR_ERR_INTERNAL_ERROR, filename, funcname, linenr, _("%s %s"), msg1, msg2); - err = 0; - cleanup: VIR_FREE(msg1); VIR_FREE(msg2); - - return err; }
static PRL_RESULT @@ -152,12 +141,12 @@ getJobResultHelper(PRL_HANDLE job, unsigned int timeout, PRL_HANDLE *result, { PRL_RESULT ret, retCode;
- if ((ret = PrlJob_Wait(job, timeout))) { + if (PRL_FAILED(ret = PrlJob_Wait(job, timeout))) { logPrlErrorHelper(ret, filename, funcname, linenr); goto cleanup; }
- if ((ret = PrlJob_GetRetCode(job, &retCode))) { + if (PRL_FAILED(ret = PrlJob_GetRetCode(job, &retCode))) { logPrlErrorHelper(ret, filename, funcname, linenr); goto cleanup; } @@ -165,17 +154,24 @@ getJobResultHelper(PRL_HANDLE job, unsigned int timeout, PRL_HANDLE *result, if (retCode) { PRL_HANDLE err_handle;
+ ret = retCode; + I think we don't need to store retCode to ret /* Sometimes it's possible to get additional error info. */ - if ((ret = PrlJob_GetError(job, &err_handle))) { + if (PRL_FAILED(ret = PrlJob_GetError(job, &err_handle))) { logPrlErrorHelper(ret, filename, funcname, linenr); goto cleanup; }
- if (logPrlEventErrorHelper(err_handle, filename, funcname, linenr)) - logPrlErrorHelper(retCode, filename, funcname, linenr); + if (PRL_FAILED(retCode = PrlEvent_GetErrCode(err_handle, &retCode))) { + logPrlErrorHelper(ret, filename, funcname, linenr); + if (PRL_ERR_NO_DATA != retCode) + logPrlError(retCode); + goto cleanup; + } + + logPrlEventErrorHelper(err_handle, filename, funcname, linenr);
PrlHandle_Free(err_handle); - ret = retCode; } else { ret = PrlJob_GetResult(job, result); if (PRL_FAILED(ret)) {
12-Aug-16 17:08, Mikhail Feoktistov пишет:
On 10.08.2016 20:44, Maxim Nestratov wrote:
First, make function logPrlEventErrorHelper be void and only print information (if any) from an event. Second, don't rewrite original error with any errors we get during parsing event info. Third, ignore PRL_ERR_NO_DATA at all.
Signed-off-by: Maxim Nestratov <mnestratov@virtuozzo.com> --- src/vz/vz_sdk.c | 32 ++++++++++++++------------------ 1 file changed, 14 insertions(+), 18 deletions(-)
diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index f81b320..45cac65 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -105,19 +105,12 @@ logPrlErrorHelper(PRL_RESULT err, const char *filename, } \ } while (0) -static PRL_RESULT +static void logPrlEventErrorHelper(PRL_HANDLE event, const char *filename, const char *funcname, size_t linenr) { - PRL_RESULT ret, retCode; char *msg1 = NULL, *msg2 = NULL; PRL_UINT32 len = 0; - int err = -1; - - if ((ret = PrlEvent_GetErrCode(event, &retCode))) { - logPrlError(ret); - return ret; - } PrlEvent_GetErrString(event, PRL_TRUE, PRL_FALSE, NULL, &len); @@ -136,13 +129,9 @@ logPrlEventErrorHelper(PRL_HANDLE event, const char *filename, virReportErrorHelper(VIR_FROM_THIS, VIR_ERR_INTERNAL_ERROR, filename, funcname, linenr, _("%s %s"), msg1, msg2); - err = 0; - cleanup: VIR_FREE(msg1); VIR_FREE(msg2); - - return err; } static PRL_RESULT @@ -152,12 +141,12 @@ getJobResultHelper(PRL_HANDLE job, unsigned int timeout, PRL_HANDLE *result, { PRL_RESULT ret, retCode; - if ((ret = PrlJob_Wait(job, timeout))) { + if (PRL_FAILED(ret = PrlJob_Wait(job, timeout))) { logPrlErrorHelper(ret, filename, funcname, linenr); goto cleanup; } - if ((ret = PrlJob_GetRetCode(job, &retCode))) { + if (PRL_FAILED(ret = PrlJob_GetRetCode(job, &retCode))) { logPrlErrorHelper(ret, filename, funcname, linenr); goto cleanup; } @@ -165,17 +154,24 @@ getJobResultHelper(PRL_HANDLE job, unsigned int timeout, PRL_HANDLE *result, if (retCode) { PRL_HANDLE err_handle; + ret = retCode; + I think we don't need to store retCode to ret
Actually returning what we get from PrlJob_GetRetCode is one the reasons of this patch. But you are right that we don't have to rewrite it later two lines below.
/* Sometimes it's possible to get additional error info. */ - if ((ret = PrlJob_GetError(job, &err_handle))) { + if (PRL_FAILED(ret = PrlJob_GetError(job, &err_handle))) {
there should have been the following: + if (PRL_FAILED(retCode = PrlJob_GetError(job, &err_handle))) { I messed things up while rebasing
logPrlErrorHelper(ret, filename, funcname, linenr); goto cleanup; } - if (logPrlEventErrorHelper(err_handle, filename, funcname, linenr)) - logPrlErrorHelper(retCode, filename, funcname, linenr); + if (PRL_FAILED(retCode = PrlEvent_GetErrCode(err_handle, &retCode))) { + logPrlErrorHelper(ret, filename, funcname, linenr); + if (PRL_ERR_NO_DATA != retCode) + logPrlError(retCode); + goto cleanup; + } + + logPrlEventErrorHelper(err_handle, filename, funcname, linenr); PrlHandle_Free(err_handle); - ret = retCode; } else { ret = PrlJob_GetResult(job, result); if (PRL_FAILED(ret)) {
There is a possibility that qemu driver frees by unreferencing its closeCallbacks pointer as it has the only reference to the object, while in fact not all users of CloseCallbacks called thier virCloseCallbacksUnset. Backtrace is the following: Thread #1: 0 in pthread_cond_wait@@GLIBC_2.3.2 () from /lib64/libpthread.so.0 1 in virCondWait (c=<optimized out>, m=<optimized out>) at util/virthread.c:154 2 in virThreadPoolFree (pool=0x7f0810110b50) at util/virthreadpool.c:266 3 in qemuStateCleanup () at qemu/qemu_driver.c:1116 4 in virStateCleanup () at libvirt.c:808 5 in main (argc=<optimized out>, argv=<optimized out>) at libvirtd.c:1660 Thread #2: 0 in virClassIsDerivedFrom (klass=0xdeadbeef, parent=0x7f0837c694d0) at util/virobject.c:169 1 in virObjectIsClass (anyobj=anyobj@entry=0x7f08101d4760, klass=<optimized out>) at util/virobject.c:365 2 in virObjectLock (anyobj=0x7f08101d4760) at util/virobject.c:317 3 in virCloseCallbacksUnset (closeCallbacks=0x7f08101d4760, vm=vm@entry=0x7f08101d47b0, cb=cb@entry=0x7f081d078fc0 <qemuProcessAutoDestroy>) at util/virclosecallbacks.c:163 4 in qemuProcessAutoDestroyRemove (driver=driver@entry=0x7f081018be50, vm=vm@entry=0x7f08101d47b0) at qemu/qemu_process.c:6368 5 in qemuProcessStop (driver=driver@entry=0x7f081018be50, vm=vm@entry=0x7f08101d47b0, reason=reason@entry=VIR_DOMAIN_SHUTOFF_SHUTDOWN, asyncJob=asyncJob@entry=QEMU_ASYNC_JOB_NONE, flags=flags@entry=0) at qemu/qemu_process.c:5854 6 in processMonitorEOFEvent (vm=0x7f08101d47b0, driver=0x7f081018be50) at qemu/qemu_driver.c:4585 7 qemuProcessEventHandler (data=<optimized out>, opaque=0x7f081018be50) at qemu/qemu_driver.c:4629 8 in virThreadPoolWorker (opaque=opaque@entry=0x7f0837c4f820) at util/virthreadpool.c:145 9 in virThreadHelper (data=<optimized out>) at util/virthread.c:206 10 in start_thread () from /lib64/libpthread.so.0 Let's reference CloseCallbacks object in virCloseCallbacksSet and unreference in virCloseCallbacksUnset. Signed-off-by: Maxim Nestratov <mnestratov@virtuozzo.com> --- src/util/virclosecallbacks.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/util/virclosecallbacks.c b/src/util/virclosecallbacks.c index 82d4071..891a92b 100644 --- a/src/util/virclosecallbacks.c +++ b/src/util/virclosecallbacks.c @@ -141,6 +141,7 @@ virCloseCallbacksSet(virCloseCallbacksPtr closeCallbacks, virObjectRef(vm); } + virObjectRef(closeCallbacks); ret = 0; cleanup: virObjectUnlock(closeCallbacks); @@ -180,6 +181,8 @@ virCloseCallbacksUnset(virCloseCallbacksPtr closeCallbacks, ret = 0; cleanup: virObjectUnlock(closeCallbacks); + if (!ret) + virObjectUnref(closeCallbacks); return ret; } -- 1.8.3.1
Signed-off-by: Maxim Nestratov <mnestratov@virtuozzo.com> --- src/vz/vz_driver.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index b1b6d14..cf4b9e8 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -600,6 +600,7 @@ static int vzDomainGetInfo(virDomainPtr domain, virDomainInfoPtr info) { virDomainObjPtr dom; + vzDomObjPtr privdom; int ret = -1; if (!(dom = vzDomObjFromDomainRef(domain))) @@ -611,13 +612,12 @@ vzDomainGetInfo(virDomainPtr domain, virDomainInfoPtr info) info->nrVirtCpu = virDomainDefGetVcpus(dom->def); info->cpuTime = 0; - if (virDomainObjIsActive(dom)) { + privdom = dom->privateData; + + if (PRL_INVALID_HANDLE != privdom->stats && virDomainObjIsActive(dom)) { unsigned long long vtime; - vzDomObjPtr privdom; size_t i; - privdom = dom->privateData; - for (i = 0; i < virDomainDefGetVcpus(dom->def); ++i) { if (prlsdkGetVcpuStats(privdom->stats, i, &vtime) < 0) { virReportError(VIR_ERR_OPERATION_FAILED, "%s", -- 1.8.3.1
If we are going to ignore return value of a functions that can raise an error, it's not enough to use ignore_value construction. We should explicitly call virResetLastError Signed-off-by: Maxim Nestratov <mnestratov@virtuozzo.com> --- src/vz/vz_driver.c | 5 +++++ src/vz/vz_sdk.c | 7 +++++++ 2 files changed, 12 insertions(+) diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index cf4b9e8..2ed12db 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -331,6 +331,11 @@ vzDriverObjNew(void) driver->hostsysinfo = virSysinfoRead(); ignore_value(prlsdkLoadDomains(driver)); + + /* As far as waitDomainJob finally calls virReportErrorHelper + * and we are not going to report it, reset it expicitly*/ + virResetLastError(); + return driver; } diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index 45cac65..02f83da 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -3113,6 +3113,9 @@ static int prlsdkConfigureGateways(PRL_HANDLE sdknet, virDomainNetDefPtr net) ? VIR_SOCKET_ADDR_IPV4_ALL : VIR_SOCKET_ADDR_IPV6_ALL), VIR_SOCKET_ADDR_FAMILY(addrdst))); + /* virSocketAddrParse raises an error + * and we are not going to report it, reset it expicitly*/ + virResetLastError(); if (!virSocketAddrEqual(addrdst, &zero)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", @@ -3359,6 +3362,10 @@ prlsdkCleanupBridgedNet(vzDriverPtr driver, job = PrlSrv_DeleteVirtualNetwork(driver->server, vnet, 0); ignore_value(waitDomainJob(job, dom)); + /* As far as waitDomainJob finally calls virReportErrorHelper + * and we are not going to report it, reset it expicitly*/ + virResetLastError(); + cleanup: PrlHandle_Free(vnet); } -- 1.8.3.1
Somehow we lost this during recent refactoring Signed-off-by: Maxim Nestratov <mnestratov@virtuozzo.com> --- src/vz/vz_sdk.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index 02f83da..7e5ec91 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -977,6 +977,7 @@ prlsdkGetNetInfo(PRL_HANDLE netAdapter, virDomainNetDefPtr net, bool isCt) /* venet devices don't have mac address and * always up */ net->linkstate = VIR_DOMAIN_NET_INTERFACE_LINK_STATE_UP; + net->type = VIR_DOMAIN_NET_TYPE_NETWORK; if (VIR_STRDUP(net->data.network.name, PARALLELS_DOMAIN_ROUTED_NETWORK_NAME) < 0) goto cleanup; @@ -1000,6 +1001,7 @@ prlsdkGetNetInfo(PRL_HANDLE netAdapter, virDomainNetDefPtr net, bool isCt) prlsdkCheckRetGoto(pret, cleanup); if (emulatedType == PNA_ROUTED) { + net->type = VIR_DOMAIN_NET_TYPE_NETWORK; if (VIR_STRDUP(net->data.network.name, PARALLELS_DOMAIN_ROUTED_NETWORK_NAME) < 0) goto cleanup; -- 1.8.3.1
10-Aug-16 20:44, Maxim Nestratov пишет:
Maxim Nestratov (5): vz: get additional error information from job correctly util: fix crash in virClassIsDerivedFrom for CloseCallbacks objects vz: don't fail query domain info in case we don't have valid stats handle vz: reset errors after ignoring return values vz: specify VIR_DOMAIN_NET_TYPE_NETWORK for routed networks
Yuri Pudgorodskiy (1): qemu: guest agent: introduce new error code VIR_ERR_AGENT_UNSYNCED
include/libvirt/virterror.h | 2 ++ src/qemu/qemu_agent.c | 6 +++--- src/util/virclosecallbacks.c | 3 +++ src/util/virerror.c | 6 ++++++ src/vz/vz_driver.c | 13 +++++++++---- src/vz/vz_sdk.c | 41 +++++++++++++++++++++++------------------ 6 files changed, 46 insertions(+), 25 deletions(-)
Resent a new version. Maxim
participants (2)
-
Maxim Nestratov -
Mikhail Feoktistov