[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v2 1/2] virsh: Switch from generated cmd*Edit commands to nongenerated



On 05/24/2012 10:20 AM, Michal Privoznik wrote:
> Currently, we either generate some cmd*Edit commands (cmdPoolEdit
> and cmdNetworkEdit) via sed script or copy the body of cmdEdit
> (e.g. cmdInterfaceEdit, cmdNWFilterEdit, etc.). This fact makes
> it harder to implement any new feature to our editing system.
> Therefore switch to new implementation - define macros to:
> - dump XML (EDIT_GET_XML)
> - take an action if XML wasn't changed,
>   usually just vshPrint() (EDIT_NOT_CHANGED)
> - define new object (EDIT_DEFINE) - the edited XML is in @doc_edited
> and #include "virsh-edit.c"
> ---
>  cfg.mk             |    4 +-
>  po/POTFILES.in     |    1 +
>  tools/Makefile.am  |   40 +-----
>  tools/virsh-edit.c |  111 ++++++++++++++
>  tools/virsh.c      |  421 +++++++++++++++++-----------------------------------
>  5 files changed, 255 insertions(+), 322 deletions(-)
>  create mode 100644 tools/virsh-edit.c

Needs a v3.  See my interleaved comments (hopefully it's obvious how I'm
pointing out two different causes and later the locations that need
fixes for those bugs).

> + *
> + * EDIT_DEFINE - which redefines the object. The edited XML from user is in
> + *      'doc_edited' variable. Don't overwrite the pointer to the object,
> + *      as we may iterate once more over and therefore the pointer would be
> + *      invalid. Hence assign object to a different variable.
> + *      Moreover, this needs to be an expression where:
> + *      - 0 is taken as error (our virDefine* APIs often return NULL on error)
> + *      - everything else is taken as success
> + *      For example:
> + *      #define EDIT_DEFINE dom_edited = virDomainDefineXML(ctl->conn, doc_edited)

This recommendation is a memory leak - if you call EDIT_DEFINE more than
once, then the second call will overwrite the dom_edited from the first
call.  The fix is to add a fourth macro, EDIT_FREE, which is called as a
statement [1]...

> + *
> + * Michal Privoznik <mprivozn redhat com>
> + */
> +
> +#ifndef EDIT_GET_XML
> +# error Missing EDIT_GET_XML definition
> +#endif
> +
> +#ifndef EDIT_NOT_CHANGED
> +# error Missing EDIT_NOT_CHANGED definition
> +#endif
> +
> +#ifndef EDIT_DEFINE
> +# error Missing EDIT_DEFINE definition
> +#endif
> +
> +do {

> +    /* Read back the edited file. */
> +    doc_edited = editReadBackFile(ctl, tmp);
> +    if (!doc_edited)
> +        goto edit_cleanup;
> +
> +    /* Compare original XML with edited.  Has it changed at all? */
> +    if (STREQ(doc, doc_edited)) {
> +        EDIT_NOT_CHANGED;
> +        ret = true;
> +        goto edit_cleanup;
> +    }

You have a bug here.  This _always_ skips the redefine phase, but at
least one caller[2]...


> +
> +    /* Now re-read the object XML.  Did someone else change it while
> +     * it was being edited?  This also catches problems such as us
> +     * losing a connection or the object going away.
> +     */
> +    doc_reread = (EDIT_GET_XML);
> +    if (!doc_reread)
> +        goto edit_cleanup;
> +
> +    if (STRNEQ(doc, doc_reread)) {
> +        vshError(ctl, "%s", _("ERROR: the XML configuration "
> +                              "was changed by another user"));
> +        goto edit_cleanup;
> +    }
> +
> +    /* Everything checks out, so redefine the domain. */

[1]...right here.

> +    if (!(EDIT_DEFINE))
> +        goto edit_cleanup;
> +
> +    break;
> +
> +edit_cleanup:
> +    VIR_FREE(doc);
> +    VIR_FREE(doc_edited);
> +    VIR_FREE(doc_reread);
> +    if (tmp) {
> +        unlink (tmp);
> +        VIR_FREE(tmp);
> +    }
> +    goto cleanup;
> +
> +} while (0);
> +
> +
> +#undef EDIT_GET_XML
> +#undef EDIT_NOT_CHANGED
> +#undef EDIT_DEFINE
> diff --git a/tools/virsh.c b/tools/virsh.c

> -    if (!tmp) goto cleanup;
> -
> -    /* Start the editor. */
> -    if (editFile (ctl, tmp) == -1) goto cleanup;
> +    #define EDIT_GET_XML virInterfaceGetXMLDesc(iface, flags)
> +    #define EDIT_NOT_CHANGED \
> +        vshPrint(ctl, _("Interface %s XML configuration not changed.\n"), \
> +                 virInterfaceGetName(iface))
> +    #define EDIT_DEFINE \
> +        iface_edited = virInterfaceDefineXML(ctl->conn, doc_edited, 0)
> +    #include "virsh-edit.c"

[1] Here, EDIT_FREE would be /* */ (no extra work, as no extra resources
are allocated).

> +    #define EDIT_GET_XML virInterfaceGetXMLDesc(iface, flags)
> +    #define EDIT_NOT_CHANGED \
> +        vshPrint(ctl, _("Interface %s XML configuration not changed.\n"), \
> +                 virInterfaceGetName(iface))
> +    #define EDIT_DEFINE \
> +        iface_edited = virInterfaceDefineXML(ctl->conn, doc_edited, 0)

Here, EDIT_FREE would be:
if (iface_edited)
    virInterfaceFree (iface_edited);

and so forth.

> -
> -    /* Compare original XML with edited.  Short-circuit if it did not
> -     * change, and we do not have any flags.  */
> -    if (STREQ(doc, doc_edited) &&
> -        !(define_flags & VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT)) {
> -        vshPrint(ctl, _("Snapshot %s XML configuration not changed.\n"),
> -                 name);
> -        ret = true;
> -        goto cleanup;
> -    }

[2]...has semantics where the presence of a flag MUST trigger a
redefine, even if the XML didn't change, but your replacement...

> -
> -    /* Everything checks out, so redefine the xml.  */
> -    edited = virDomainSnapshotCreateXML(dom, doc_edited, define_flags);
> -    if (!edited) {
> -        vshError(ctl, _("Failed to update %s"), name);
> -        goto cleanup;
> -    }
> +    #define EDIT_GET_XML \
> +        virDomainSnapshotGetXMLDesc(snapshot, getxml_flags)
> +    #define EDIT_NOT_CHANGED \
> +        if (define_flags & VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT) \
> +            goto cleanup; \
> +        vshPrint(ctl, _("Snapshot %s XML configuration not changed.\n"), name)

[2]...is not following those semantics.  Perhaps the right fix is to
change 'EDIT_NOT_CHANGED' to have the caller be in control of issuing
'ret=true; goto cleanup;' as appropriate, rather than blindly doing that
for all callers in the include file.  Then this particular definition
would be:

#define EDIT_NOT_CHANGED \
    /* Depending on flags, we re-edit even if XML is unchanged.  */ \
    if (!define_flags & VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT) {       \
        vshPrint(ctl,                                               \
                 _("Snapshot %s XML configuration not changed.\n"), \
                 name);                                             \
        ret = true;                                                 \
        goto cleanup;                                               \
    }

while most other definitions skip the if(){} and just have the three
statements.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]