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@xxxxxxxxxxxxxxxxxx>
> ---
> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list