[libvirt] [PATCH 1/2] virsh: Introduce virshDomainDetachInterface function.

[Cole Robinson]

On 04/28/2016 05:11 AM, Nitesh Konkar wrote:
> virshDomainDetachInterface handles virsh interface
> detach from the specified live/config domain xml.
> 
> Signed-off-by: Nitesh Konkar <nitkon12 at linux.vnet.ibm.com>
> ---
>  tools/virsh-domain.c | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 99 insertions(+)
> 
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index 36d0353..7cb042b 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -11118,6 +11118,105 @@ static const vshCmdOptDef opts_detach_interface[] = {
>  };
>  

I meant that this patch should also be removing the functionality
cmdDetachInterface at the same time. So patch #1: break out the old
functionality to virshDomainDetachInterface, and use it in cmdDetachInterface.
The change should be a functional no-op

Then patch #2 changes the behavior. The point is that you isolate the
functional changes in the smallest patch possible, since that is the harder
stuff to review, but a smaller patch will make it easier to review.

>  static bool
> + virshDomainDetachInterface(char *doc, unsigned int flags, virDomainPtr dom, vshControl *ctl, const vshCmd *cmd)
> +{

This line is too long. Split it like:

static bool
virshDomainDetachInterface(char *doc,
                           unsigned int flags,
                           ...

> +    xmlDocPtr xml = NULL;
> +    xmlXPathObjectPtr obj = NULL;
> +    xmlXPathContextPtr ctxt = NULL;
> +    xmlNodePtr cur = NULL, matchNode = NULL;
> +    const char *mac = NULL, *type = NULL;
> +    char *detach_xml = NULL, buf[64];
> +    bool current = vshCommandOptBool(cmd, "current");
> +    int diff_mac,ret;

The style is wrong here, I see you fixed it in patch 2, but the change should
have been in this patch. Make sure every patch is syntax-check clean

Please fix those issues and repost

Thanks,
Cole