7:24 a.m.
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
7:24 a.m.
New subject: [libvirt] [PATCH v2 1/7] qemu: guest agent: introduce new error code VIR_ERR_AGENT_UNSYNCED
From: Yuri Pudgorodskiy <yur(a)virtuozzo.com>
A separate error code will help recognize real failures from
necessity to try again
Signed-off-by: Maxim Nestratov <mnestratov(a)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
7:24 a.m.
New subject: [libvirt] [PATCH v2 2/7] vz: get additional error information from job correctly
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(a)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
7:24 a.m.
New subject: [libvirt] [PATCH v2 3/7] util: fix crash in virClassIsDerivedFrom for CloseCallbacks objects
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@(a)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(a)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
7:24 a.m.
New subject: [libvirt] [PATCH v2 4/7] vz: don't fail query domain info in case we don't have valid stats handle
Signed-off-by: Maxim Nestratov <mnestratov(a)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
7:24 a.m.
New subject: [libvirt] [PATCH v2 5/7] vz: reset errors after ignoring return values
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(a)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
7:24 a.m.
New subject: [libvirt] [PATCH v2 6/7] vz: specify VIR_DOMAIN_NET_TYPE_NETWORK for routed networks
Somehow we lost this during recent refactoring
Signed-off-by: Maxim Nestratov <mnestratov(a)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
7:24 a.m.
New subject: [libvirt] [PATCH v2 7/7] vz: relax disk bus controller and device indices check
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(a)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
5:41 a.m.
New subject: [libvirt] [PATCH v2 7/7] vz: relax disk bus controller and device indices check
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(a)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;
}
5:43 a.m.
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(-)
8:12 a.m.
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
12:47 p.m.
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
3018
days inactive
3020
days old
11 comments
2 participants
participants (2)
-
Maxim Nestratov -
Mikhail Feoktistov