[libvirt] [PATCH 4/8] qemu: Cleanup improper VIR_ERR_NO_SUPPORT use

[Osier Yang]

于 2011年08月25日 05:47, Daniel P. Berrange 写道:
> On Tue, Aug 23, 2011 at 05:39:41PM +0800, Osier Yang wrote:
>> * src/qemu/qemu_command.c: s/VIR_ERR_NO_SUPPORT/VIR_ERR_CONFIG_UNSUPPORTED/
>>
>> * src/qemu/qemu_driver.c: s/VIR_ERR_NO_SUPPORT/VIR_ERR_OPERATION_INVALID/
>>
>> * src/qemu/qemu_process.c: s/VIR_ERR_NO_SUPPORT/VIR_ERR_OPERATION_INVALID/
>> ---
>>   src/qemu/qemu_command.c |    4 ++--
>>   src/qemu/qemu_driver.c  |   16 ++++++++--------
>>   src/qemu/qemu_process.c |    4 ++--
>>   3 files changed, 12 insertions(+), 12 deletions(-)
>>
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index dbfc7d9..287ad8d 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -4084,7 +4084,7 @@ qemuBuildCommandLine(virConnectPtr conn,
>>           switch(console->targetType) {
>>           case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO:
>>               if (!qemuCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) {
>> -                qemuReportError(VIR_ERR_NO_SUPPORT, "%s",
>> +                qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>>                       _("virtio channel requires QEMU to support -device"));
>>                   goto error;
>>               }
>> @@ -4109,7 +4109,7 @@ qemuBuildCommandLine(virConnectPtr conn,
>>               break;
>>
>>           default:
>> -            qemuReportError(VIR_ERR_NO_SUPPORT,
>> +            qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>>                               _("unsupported console target type %s"),
>>                               NULLSTR(virDomainChrConsoleTargetTypeToString(console->targetType)));
>>               goto error;
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index c8dda73..fc2538a 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -1552,7 +1552,7 @@ static int qemuDomainReboot(virDomainPtr dom, unsigned int flags) {
>>               vm = NULL;
>>       } else {
>>   #endif
>> -        qemuReportError(VIR_ERR_NO_SUPPORT, "%s",
>> +        qemuReportError(VIR_ERR_OPERATION_INVALID, "%s",
>>                           _("Reboot is not supported without the JSON monitor"));
>>   #if HAVE_YAJL
>>       }
>> @@ -3309,7 +3309,7 @@ qemudDomainPinVcpuFlags(virDomainPtr dom,
>>                                             cpumap, maplen, maxcpu)<  0)
>>                   goto cleanup;
>>           } else {
>> -            qemuReportError(VIR_ERR_NO_SUPPORT,
>> +            qemuReportError(VIR_ERR_OPERATION_INVALID,
>>                               "%s", _("cpu affinity is not supported"));
>>               goto cleanup;
>>           }
>> @@ -3563,7 +3563,7 @@ qemudDomainGetVcpus(virDomainPtr dom,
>>                           goto cleanup;
>>                   }
>>               } else {
>> -                qemuReportError(VIR_ERR_NO_SUPPORT,
>> +                qemuReportError(VIR_ERR_OPERATION_INVALID,
>>                                   "%s", _("cpu affinity is not available"));
>>                   goto cleanup;
>>               }
>> @@ -5637,7 +5637,7 @@ static int qemuDomainSetBlkioParameters(virDomainPtr dom,
>>           }
>>
>>           if (!qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_BLKIO)) {
>> -            qemuReportError(VIR_ERR_NO_SUPPORT, _("blkio cgroup isn't mounted"));
>> +            qemuReportError(VIR_ERR_OPERATION_INVALID, _("blkio cgroup isn't mounted"));
>>               goto cleanup;
>>           }
>>
>> @@ -5790,7 +5790,7 @@ static int qemuDomainGetBlkioParameters(virDomainPtr dom,
>>           }
>>
>>           if (!qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_BLKIO)) {
>> -            qemuReportError(VIR_ERR_NO_SUPPORT, _("blkio cgroup isn't mounted"));
>> +            qemuReportError(VIR_ERR_OPERATION_INVALID, _("blkio cgroup isn't mounted"));
>>               goto cleanup;
>>           }
>>
> THe use of VIR_ERR_OPERATION_INVALID is not correct here, but I'm not
> certain what other error is best. Perhaps ARGUMENT_UNSUPPORTED

For VIR_ERR_OPERATION_INVALID:

     errmsg = _("Requested operation is not valid");

For VIR_ERR_ARGUMENT_UNSUPPORTED:

     errmsg = _("argument unsupported");

 From the user's point of view, I think OPERATION_INVALID is more clear
and sensiable. E.g.
    "Requested operation is not valid: blkio cgroup isn't mounted"

    "argument unsupported: blkio cgroup isn't mounted"

Could we just extend the meaning for OPERATION_INVALID
so that it's not just used when the object operated on is not in
correct state, and used for things like above?

Otherwise, I'd prefer INTERNAL_ERROR here.

>
>> @@ -6887,8 +6887,8 @@ qemudDomainInterfaceStats (virDomainPtr dom,
>>                              const char *path ATTRIBUTE_UNUSED,
>>                              struct _virDomainInterfaceStats *stats ATTRIBUTE_UNUSED)
>>   {
>> -    qemuReportError(VIR_ERR_NO_SUPPORT,
>> -                    "%s", __FUNCTION__);
>> +    qemuReportError(VIR_ERR_OPERATION_INVALID, "%s",
>> +                    _("interface stats not implemented on this platform"));
>>       return -1;
>>   }
>>   #endif
>
> The original code was correct here.
>
>
>> @@ -8004,7 +8004,7 @@ qemuCPUCompare(virConnectPtr conn,
>>       qemuDriverLock(driver);
>>
>>       if (!driver->caps || !driver->caps->host.cpu) {
>> -        qemuReportError(VIR_ERR_NO_SUPPORT,
>> +        qemuReportError(VIR_ERR_OPERATION_INVALID,
>>                           "%s", _("cannot get host CPU capabilities"));
>>       } else {
>>           ret = cpuCompareXML(driver->caps->host.cpu, xmlDesc);
> The use of OPERATION_INVALID is not correct here. Perhaps
> ARGUMENT_UNSUPPORTED

perhaps INTERNAL_ERROR is better here, as translated error string
for ARGUMENT_UNSUPPORTED is quite confused.

>
>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>> index 6f54b30..616c8e2 100644
>> --- a/src/qemu/qemu_process.c
>> +++ b/src/qemu/qemu_process.c
>> @@ -261,7 +261,7 @@ qemuProcessGetVolumeQcowPassphrase(virConnectPtr conn,
>>       if (conn->secretDriver == NULL ||
>>           conn->secretDriver->lookupByUUID == NULL ||
>>           conn->secretDriver->getValue == NULL) {
>> -        qemuReportError(VIR_ERR_NO_SUPPORT, "%s",
>> +        qemuReportError(VIR_ERR_OPERATION_INVALID, "%s",
>>                           _("secret storage not supported"));
>>           goto cleanup;
>>       }
> NO_SUPPORT was the right value here.
>
>> @@ -1464,7 +1464,7 @@ qemuProcessSetVcpuAffinites(virConnectPtr conn,
>>           return 0;
>>
>>       if (priv->vcpupids == NULL) {
>> -        qemuReportError(VIR_ERR_NO_SUPPORT,
>> +        qemuReportError(VIR_ERR_OPERATION_INVALID,
>>                           "%s", _("cpu affinity is not supported"));
>>           return -1;
>>       }
> Perhaps ARGUMENT_UNSUPPORTED

Perhaps INTERNAL_ERROR?

>
> Daniel