Re: [libvirt] [PATCH 1/2] virsh: Introduce virshDomainDetachInterface function.
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(a)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