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

Re: [libvirt] [PATCH 2/2] virsh: Pass the corect live/config xml to virshDomainDetachInterface.



On 05/02/2016 09:59 AM, Nitesh Konkar wrote:
> cmdDetachInterface function checks for live config
> flags and then passes the live/config domain xml
> to virshDomainDetachInterface accordingly.
> 
> Signed-off-by: Nitesh Konkar <nitkon12 linux vnet ibm com>
> ---
>  tools/virsh-domain.c | 32 +++++++++++++++++++-------------
>  1 file changed, 19 insertions(+), 13 deletions(-)
> 
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index d9fde4d..5cfd6a3 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -11302,7 +11302,7 @@ static bool
>  cmdDetachInterface(vshControl *ctl, const vshCmd *cmd)
>  {
>      virDomainPtr dom = NULL;
> -    char *doc = NULL;
> +    char *doc_live = NULL, *doc_config = NULL;
>      bool functionReturn = false;
>      unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT;
>      bool current = vshCommandOptBool(cmd, "current");
> @@ -11317,8 +11317,6 @@ cmdDetachInterface(vshControl *ctl, const vshCmd *cmd)
>  

Hmm, I'm having trouble following the logic here with the flag manipulation. I
think there's an error here with --persistent handling at least. I suggest to
drop the 'flags' variable entirely. So...

>      if (config || persistent)
>          flags |= VIR_DOMAIN_AFFECT_CONFIG;

Here I'd change to

bool affect_config;
...
affect_config = (config || persistent);

> -    if (live)
> -        flags |= VIR_DOMAIN_AFFECT_LIVE;
>  
>      if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
>          return false;
> @@ -11327,15 +11325,23 @@ cmdDetachInterface(vshControl *ctl, const vshCmd *cmd)
>          virDomainIsActive(dom) == 1)
>          flags |= VIR_DOMAIN_AFFECT_LIVE;
>  

> -    if (flags & VIR_DOMAIN_AFFECT_CONFIG)
> -        doc = virDomainGetXMLDesc(dom, VIR_DOMAIN_XML_INACTIVE);
> -    else
> -        doc = virDomainGetXMLDesc(dom, 0);
> +    if (flags & VIR_DOMAIN_AFFECT_CONFIG) {

This will just be (affect_config)

> +        if (!(doc_config = virDomainGetXMLDesc(dom, VIR_DOMAIN_XML_INACTIVE)))
> +            goto cleanup;
> +        if (!(functionReturn = virshDomainDetachInterface(doc_config, flags, dom, ctl, cmd)))
> +            goto cleanup;
> +    }
>  
> -    if (!doc)
> -        goto cleanup;
> -    else
> -       functionReturn = virshDomainDetachInterface(doc, flags, dom, ctl, cmd);
> +    flags = 0;
> +
> +    if (live || (!live && !config))
> +        flags |= VIR_DOMAIN_AFFECT_LIVE;
> +
> +    if (flags & VIR_DOMAIN_AFFECT_LIVE) {

This will be:

bool affect_live;
...
affect_live = (live || (persistent && virDomainIsActive(dom) == 1));
if (affect_live || !affect_config)
   int flags = 0;
   if (affect_live)
       flags |= VIR_DOMAIN_AFFECT_LIVE;
   ...

The last bit with the 'int flags' ensures we still use flags=0 and old style
virDomainDetachDevice() if the user doesn't specify any command line flags. We
want to preserve that behavior for compat with older libvirt

- Cole


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