On 26.10.2016 23:04, John Ferlan wrote:
>
>
> On 10/04/2016 02:56 AM, Nikolay Shirokovskiy wrote:
>> agentError is used for 2 different cases:
>>
>> 1. agent monitor is failed to start
>
> Non guest fatal failure in qemuConnectAgent when first trying to connect
> to the agent
>
>> 2. io error in agent monitor
>
> I/O error with running agent resulting in qemuProcessHandleAgentError
> being called
>
> Most of the above would seemingly be a reason for the removal of the
> agentError (which needs to be a separate patch)...
Removal is done in 2 steps. This is a step 1. Drop
flag usage for the case 1.
>
>>
>> Actually to check for the first case we don't need the
>> flag at all. NULL agent is always error for old qemu
>> (QEMU_CAPS_VSERPORT_CHANGE is not supported), while
>> for modern qemu it is an error only if channel is in
>> connected state.
>
> I suppose this explanation could be added too, but I'd have to see it in
> the context of what I've learned so far in a "new" series.
>
> In any case, none of the above text seems to have anything to do with
> the change in reporting the errors at startup.
'failed agent start case' - that is case 1, we drop flag usage for
this case. Nothing to do... how come?
>
>> ---
>> src/qemu/qemu_domain.c | 37 +++++++++++++++++++++++--------------
>> src/qemu/qemu_process.c | 1 -
>> 2 files changed, 23 insertions(+), 15 deletions(-)
>>
>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> index 9b1a32e..cd788c8 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -5046,6 +5046,7 @@ qemuDomainAgentAvailable(virDomainObjPtr vm,
>> bool reportError)
>> {
>> qemuDomainObjPrivatePtr priv = vm->privateData;
>> + virDomainChrDefPtr config;
>>
>> if (virDomainObjGetState(vm, NULL) != VIR_DOMAIN_RUNNING) {
>> if (reportError) {
>> @@ -5062,22 +5063,30 @@ qemuDomainAgentAvailable(virDomainObjPtr vm,
>> }
>> return false;
>> }
>> - if (!priv->agent) {
>> - if (qemuFindAgentConfig(vm->def)) {
>> - if (reportError) {
>> - virReportError(VIR_ERR_AGENT_UNRESPONSIVE, "%s",
>> - _("QEMU guest agent is not connected"));
>> - }
>> - return false;
>> - } else {
>> - if (reportError) {
>> - virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
>> - _("QEMU guest agent is not configured"));
>> - }
>> - return false;
>> +
>> + if (priv->agent)
>> + return true;
>> +
>> + config = qemuFindAgentConfig(vm->def);
>> +
>> + if (!config) {
>> + if (reportError) {
>> + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
>> + _("QEMU guest agent is not configured"));
>> }
>
> I'm OK through here, but the next two I'm not so sure I agree with.
>
>> + } else if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VSERPORT_CHANGE) &&
>> + config->state != VIR_DOMAIN_CHR_DEVICE_STATE_CONNECTED) {
>
> What does the manner of startup (either legacy or VSERPORT) have to do
> with which error should be reported.
Ok. Let me explain it further.
Case 1. Legacy.
1. domain start (migration finish etc)
2. qemuConnectAgent is called, failed
we need to report 'agent error'
Case 2. VSERPORT
1. domain start
2. qemuConnectAgent exits early
in between we need to report 'not connected'
3. VSERPORT_CHANGE arrived
4. qemuConnectAgent is called, failed
now we need to report 'agent error'
>> + if (reportError) {
>> + virReportError(VIR_ERR_AGENT_UNRESPONSIVE, "%s",
>> + _("QEMU guest agent is not connected"));
>> + }
>> + } else if (reportError) {
>> + virReportError(VIR_ERR_AGENT_UNRESPONSIVE, "%s",
>> + _("QEMU guest agent is not "
>> + "available due to an error"));
>
> I know you're setting up for the next set of patches (to remove the
> agentError flag), but I'm not sure yet whether using VSERPORT change
> configuration is the right way.
>
> Unless of course my assumption from patch 1 review is true... This is
> really a startup/shutdown race...
>
> John
>> }
>> - return true;
>> +
>> + return false;
>> }
>>
>>
>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>> index d7c9ce3..78d530f 100644
>> --- a/src/qemu/qemu_process.c
>> +++ b/src/qemu/qemu_process.c
>> @@ -267,7 +267,6 @@ qemuConnectAgent(virQEMUDriverPtr driver, virDomainObjPtr vm)
>> cleanup:
>> if (!priv->agent) {
>> VIR_WARN("Cannot connect to QEMU guest agent for %s", vm->def->name);
>> - priv->agentError = true;
>
> Wait, what, why? you just added this in the previous patch and I see no
> reason for removing it now.
After the above changes to qemuDomainAgentAvailable the meaning of this flag
to report agent startup errors (case 1 from the commit message) is disappered.
So we don't need to set this flag anymore here. We don't add this flag in the
previous patch just move into single place.
>
> This would need a separate patch and reason.
>
I think this is a matter of one patch. We drop the use case 1 from the flag.
Nikolay
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list