libvirt-cim-bounces@redhat.com wrote on 2008-06-25 22:37:55:

> yunguol@cn.ibm.com wrote:
> > # HG changeset patch
> > # User Guolian Yun <yunguol@cn.ibm.com>
> > # Date 1214371183 -28800
> > # Node ID c8c27374e304a66cd71d1f0b5d0fc462e230a898
> > # Parent  727d97c09d77d73f3542ba49a9dd19ba119a67eb
> > [TEST] Update RAFP.01 for LXC support
> >
>
> Can you keep the revision number in the subject next time around?
> Having the revision number makes it easier to keep track of which patch
> is the current version.
>
> Also, please include a log of what has changed for each iteration of the
> patch.
>
  Sure. I will keep the revision number and log the changes for each patch.
  Thanks a lot for your great comments during the iteration of RAFP.01 patch.
  Hopefully, the latest #7 revision works well.

 
> Thanks!
>
> > +def verify_rasd(server, assoc_cn, cn, virt, list, rasd):
> > +    try:
> > +        data = assoc.AssociatorNames(server,
> > +                                     assoc_cn,
> > +                                     cn,
> > +                                     virt,
> > +                                     InstanceID=list)
> > +    except Exception:
> > +        logger.error(Globals.CIM_ERROR_ASSOCIATORNAMES % cn)
> > +        return FAIL
> > +
> > +    if len(data) < 1:
> > +        logger.error("Return NULL, expect at least one instance")
> > +        return FAIL
> > +    
> > +    for i in range(0, len(data)):
> > +        if data[i].classname == "LXC_MemResourceAllocationSettingData" \
> > +                   and data[i]['InstanceID'] == rasd["MemoryPool"]:
> > +            logger.info("MemoryPool InstanceID match")
> > +            return PASS
> > +        elif i == len(data) and data[i]['InstanceID'] !=
> rasd["MemoryPool"]:
> > +            logger.error("InstanceID Mismatch, expect %s not %s" % \
> > +                         (rasd['MemoryPool'], data[i]['InstanceID']))
> > +            return FAIL
> > +        if data[i].classname == "LXC_ProcResourceAllocationSettingData" \
> > +                   and data[i]['InstanceID'] == rasd["ProcessorPool"]:
> > +            logger.info("ProcessorPool InstanceID match")
> > +            return PASS
> > +        elif i == len(data) and data[i]['InstanceID'] !=
> rasd["ProcessorPool"]:
> > +            logger.error("InstanceID Mismatch, expect %s not %s" % \
> > +                         (rasd['ProcessorPool'], data[i]['InstanceID']))
> > +            return FAIL
> > +        if data[i].classname == "LXC_DiskResourceAllocationSettingData" \
> > +                   and data[i]['InstanceID'] == rasd["DiskPool"]:
> > +            logger.info("DiskPool InstanceID match")
> > +            return PASS
> > +        elif i == len(data) and data[i]['InstanceID'] != rasd["DiskPool"]:
> > +            logger.error("InstanceID Mismatch, expect %s" % \
> > +                         (rasd['DiskPool'], data[i]['InstanceID']))
> > +            return FAIL
> > +        if data[i].classname == "LXC_NetResourceAllocationSettingData" \
> > +                   and data[i]['InstanceID'] == rasd["NetworkPool"]:
> > +            logger.info("NetworkPool InstanceID match")
> > +            return PASS
> > +        elif i == len(data) and data[i]['InstanceID'] !=
> rasd["NetworkPool"]:
> > +            logger.error("InstanceID Mismatch, expect %s" % \
> > +                         (rasd['NetworkPool'], data[i]['InstanceID']))
> > +            return FAIL
> > +              
>
> This is a lot of duplicated code and is difficult to read.  Also, code
> like this can be difficult to maintain.  This can be collapsed into
> something much simpler:
>
>      for item in data:
>              if item['InstanceID'] == rasd[cn]:
>                  logger.info("%s InstanceID match - expected %s, got %s"
> % (cn,
>                              rasd[cn], item['InstanceID']))
>                  return PASS
>
>      logger.error("RASD instance with InstanceID %s not found" % rasd[cn])
>
>      return FAIL
>
>
> >
> > +    for k, v in pool.iteritems():
> > +        status, inst = get_instance(options.ip, k, v, options.virt)
> > +        if status != PASS:
> > +            cleanup_restore(options.ip, options.virt)
> > +            vsxml.undefine(options.ip)
> > +            return status
>
> Instead of duplicating the cleanup code here, you can use break instead.
>   That will drop you out of the for loop, and the rest of the code will
> execute normally.
>
> > +
> > +        status = verify_rasd(options.ip, "ResourceAllocationFromPool",
> > +                             k, options.virt, inst.InstanceID,
> > +                             rasd)
> > +        if status != PASS:
> > +            cleanup_restore(options.ip, options.virt)
> > +            vsxml.undefine(options.ip)
> > +            return status
>
> Same here - use break instead.
>
> > +
> > +
> > +    cleanup_restore(options.ip, options.virt)
> > +    vsxml.undefine(options.ip)
> >      return status
> >          
>
> --
> Kaitlin Rupert
> IBM Linux Technology Center
> kaitlin@linux.vnet.ibm.com
>
> _______________________________________________
> Libvirt-cim mailing list
> Libvirt-cim@redhat.com
> https://www.redhat.com/mailman/listinfo/libvirt-cim