[libvirt] [PATCH 3/4] qemu: Implement virDomainFSTrim

[Peter Krempa]

On 11/20/12 19:47, Michal Privoznik wrote:
> using qemu guest agent. As said in previous patch,
> @mountPoint must be NULL and @flags zero because
> qemu guest agent doesn't support these arguments
> yet. If qemu learns them, we can start supporting
> them as well.
> ---
>   src/qemu/qemu_agent.c  |   25 ++++++++++++++
>   src/qemu/qemu_agent.h  |    2 +
>   src/qemu/qemu_driver.c |   83 ++++++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 110 insertions(+), 0 deletions(-)
>
> diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
> index ab6dc22..6f517c9 100644
> --- a/src/qemu/qemu_agent.c
> +++ b/src/qemu/qemu_agent.c
> @@ -1448,3 +1448,28 @@ qemuAgentArbitraryCommand(qemuAgentPtr mon,
>       virJSONValueFree(reply);
>       return ret;
>   }
> +
> +int
> +qemuAgentFSTrim(qemuAgentPtr mon,
> +                unsigned long long minimum)
> +{
> +    int ret = -1;
> +    virJSONValuePtr cmd;
> +    virJSONValuePtr reply = NULL;
> +
> +    cmd = qemuAgentMakeCommand("guest-fstrim",
> +                               "U:minimum", minimum,
> +                               NULL);
> +    if (!cmd)
> +        return ret;
> +
> +    ret = qemuAgentCommand(mon, cmd, &reply,
> +                           VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK);

I didn't check the full call graph of qemuAgentCommand, but most of the 
paths already contain error reporting ... (relevance of this command 
will be revealed later :))

> +
> +    if (reply && ret == 0)
> +        ret = qemuAgentCheckError(cmd, reply);
> +
> +    virJSONValueFree(cmd);
> +    virJSONValueFree(reply);
> +    return ret;
> +}
> diff --git a/src/qemu/qemu_agent.h b/src/qemu/qemu_agent.h
> index c62c438..dad068b 100644
> --- a/src/qemu/qemu_agent.h
> +++ b/src/qemu/qemu_agent.h
> @@ -83,4 +83,6 @@ int qemuAgentArbitraryCommand(qemuAgentPtr mon,
>                                 const char *cmd,
>                                 char **result,
>                                 int timeout);
> +int qemuAgentFSTrim(qemuAgentPtr mon,
> +                    unsigned long long minimum);
>   #endif /* __QEMU_AGENT_H__ */
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 595c452..3dc5bdd 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -14733,6 +14733,88 @@ cleanup:
>       return result;
>   }
>
> +static int
> +qemuDomainFSTrim(virDomainPtr dom,
> +                 const char *mountPoint,
> +                 unsigned long long minimum,
> +                 unsigned int flags)
> +{
> +    struct qemud_driver *driver = dom->conn->privateData;
> +    virDomainObjPtr vm;
> +    int ret = -1;
> +    qemuDomainObjPrivatePtr priv;
> +
> +    virCheckFlags(0, -1);
> +
> +    if (mountPoint) {
> +        virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
> +                       _("Specifying mount point "
> +                         "is not supported for now"));
> +        return -1;
> +    }
> +
> +    qemuDriverLock(driver);
> +    vm = virDomainFindByUUID(&driver->domains, dom->uuid);
> +    qemuDriverUnlock(driver);
> +
> +    if (!vm) {
> +        char uuidstr[VIR_UUID_STRING_BUFLEN];
> +        virUUIDFormat(dom->uuid, uuidstr);
> +        virReportError(VIR_ERR_NO_DOMAIN,
> +                        _("no domain with matching uuid '%s'"), uuidstr);
> +        goto cleanup;
> +    }

You can avoid the two blocks of code above using the 
qemuDomObjFromDomain() helper that does that for you.

> +
> +    priv = vm->privateData;
> +
> +    if (!virDomainObjIsActive(vm)) {
> +        virReportError(VIR_ERR_OPERATION_INVALID,
> +                       "%s", _("domain is not running"));
> +        goto cleanup;
> +    }
> +
> +    if (priv->agentError) {
> +        virReportError(VIR_ERR_AGENT_UNRESPONSIVE, "%s",
> +                       _("QEMU guest agent is not "
> +                         "available due to an error"));
> +        goto cleanup;
> +    }
> +
> +    if (!priv->agent) {
> +        virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
> +                       _("QEMU guest agent is not configured"));
> +        goto cleanup;
> +    }

I'd swap the two blocks above, it makes more sense to check if there's 
an agent error after you check for the agent.

> +
> +    if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
> +        goto cleanup;
> +
> +    if (!virDomainObjIsActive(vm)) {
> +        virReportError(VIR_ERR_OPERATION_INVALID,
> +                       "%s", _("domain is not running"));
> +        goto endjob;
> +    }
> +
> +    qemuDomainObjEnterAgent(driver, vm);
> +    ret = qemuAgentFSTrim(priv->agent, minimum);
> +    qemuDomainObjExitAgent(driver, vm);
> +    if (ret < 0) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("Failed to execute agent command"));
> +        goto endjob;

(the relevant part from the comment up)
... this masks the error from the underlying helper function. The goto 
statement is redundant here.

> +    }
> +
> +endjob:
> +    if (qemuDomainObjEndJob(driver, vm) == 0) {

block quotes are not needed here

> +        vm = NULL;
> +    }
> +
> +cleanup:
> +    if (vm)
> +        virDomainObjUnlock(vm);
> +    return ret;
> +}
> +
>   static virDriver qemuDriver = {
>       .no = VIR_DRV_QEMU,
>       .name = QEMU_DRIVER_NAME,
> @@ -14906,6 +14988,7 @@ static virDriver qemuDriver = {
>       .nodeGetMemoryParameters = nodeGetMemoryParameters, /* 0.10.2 */
>       .nodeSetMemoryParameters = nodeSetMemoryParameters, /* 0.10.2 */
>       .nodeGetCPUMap = nodeGetCPUMap, /* 1.0.0 */
> +    .domainFSTrim = qemuDomainFSTrim, /* 1.0.1 */
>   };
>

I don't like masking errors from underlying layers. I'd rather have it 
changed so that more specific errors are shown but it's not a hard 
requirement if you think it's not needed.