[libvirt] [PATCH v2 0/7] qemu: util: vz: some fixes and improvements
v1-v2 changes: - fixed "vz: reset errors after ignoring return values" - added "vz: relax disk bus controller and device indices check" Maxim Nestratov (6): 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 vz: relax disk bus controller and device indices check 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 | 42 ++++++++++++++++++++++++------------------ src/vz/vz_utils.c | 8 -------- 7 files changed, 47 insertions(+), 33 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 | 33 +++++++++++++++------------------ 1 file changed, 15 insertions(+), 18 deletions(-) diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index f81b320..952008e 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,25 @@ 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(retCode = 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); + PrlHandle_Free(err_handle); + 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
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 952008e..999fce1 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -3114,6 +3114,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", @@ -3360,6 +3363,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 999fce1..2363f5c 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -978,6 +978,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; @@ -1001,6 +1002,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
We should not be too strict regarding device indices check because it is not really necessary and in fact it breaks some common scenarios when CD-ROMs are added as the only deivce on IDE bus with /dev/hdd device name, which corresponds to ide0-1-1 device indices, rather than ide0-0-0(hda). Signed-off-by: Maxim Nestratov <mnestratov@virtuozzo.com> --- src/vz/vz_utils.c | 8 -------- 1 file changed, 8 deletions(-) diff --git a/src/vz/vz_utils.c b/src/vz/vz_utils.c index 312355d..c707309 100644 --- a/src/vz/vz_utils.c +++ b/src/vz/vz_utils.c @@ -279,14 +279,6 @@ vzCheckDiskAddressDriveUnsupportedParams(virDomainDiskDefPtr disk) return -1; } - if (busIdx != drive->bus || devIdx != drive->unit) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Invalid drive address of disk %s, vz driver " - "does not support non default name mappings."), - disk->dst); - return -1; - } - return 0; } -- 1.8.3.1
I think there is a better solution, we should move our checks from postParse callback to validateCallback. I'm doing patch series now, so we can discuss these changes a bit later. On 16.08.2016 15:24, Maxim Nestratov wrote:
We should not be too strict regarding device indices check because it is not really necessary and in fact it breaks some common scenarios when CD-ROMs are added as the only deivce on IDE bus with /dev/hdd device name, which corresponds to ide0-1-1 device indices, rather than ide0-0-0(hda).
Signed-off-by: Maxim Nestratov <mnestratov@virtuozzo.com> --- src/vz/vz_utils.c | 8 -------- 1 file changed, 8 deletions(-)
diff --git a/src/vz/vz_utils.c b/src/vz/vz_utils.c index 312355d..c707309 100644 --- a/src/vz/vz_utils.c +++ b/src/vz/vz_utils.c @@ -279,14 +279,6 @@ vzCheckDiskAddressDriveUnsupportedParams(virDomainDiskDefPtr disk) return -1; }
- if (busIdx != drive->bus || devIdx != drive->unit) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Invalid drive address of disk %s, vz driver " - "does not support non default name mappings."), - disk->dst); - return -1; - } - return 0; }
Ack the series, except [PATCH v2 7/7] vz: relax disk bus controller and device indices check On 16.08.2016 15:24, Maxim Nestratov wrote:
v1-v2 changes: - fixed "vz: reset errors after ignoring return values" - added "vz: relax disk bus controller and device indices check"
Maxim Nestratov (6): 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 vz: relax disk bus controller and device indices check
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 | 42 ++++++++++++++++++++++++------------------ src/vz/vz_utils.c | 8 -------- 7 files changed, 47 insertions(+), 33 deletions(-)
17-Aug-16 13:43, Mikhail Feoktistov пишет:
Ack the series, except [PATCH v2 7/7] vz: relax disk bus controller and device indices check
On 16.08.2016 15:24, Maxim Nestratov wrote:
v1-v2 changes: - fixed "vz: reset errors after ignoring return values" - added "vz: relax disk bus controller and device indices check"
Maxim Nestratov (6): 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 vz: relax disk bus controller and device indices check
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 | 42 ++++++++++++++++++++++++------------------ src/vz/vz_utils.c | 8 -------- 7 files changed, 47 insertions(+), 33 deletions(-)
Ok. Thanks. I'll push vz related changes then, but I need some more eyes on qemu patches. Maxim
17-Aug-16 16:12, Maxim Nestratov пишет:
17-Aug-16 13:43, Mikhail Feoktistov пишет:
Ack the series, except [PATCH v2 7/7] vz: relax disk bus controller and device indices check
On 16.08.2016 15:24, Maxim Nestratov wrote:
v1-v2 changes: - fixed "vz: reset errors after ignoring return values" - added "vz: relax disk bus controller and device indices check"
Maxim Nestratov (6): 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 vz: relax disk bus controller and device indices check
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 | 42 ++++++++++++++++++++++++------------------ src/vz/vz_utils.c | 8 -------- 7 files changed, 47 insertions(+), 33 deletions(-)
Ok. Thanks. I'll push vz related changes then, but I need some more eyes on qemu patches.
Maxim
vz series pushed now. Thanks. Maxim
participants (2)
-
Maxim Nestratov -
Mikhail Feoktistov