On Thu, 2012-11-22 at 11:47 -0200, Lucas Meneghel Rodrigues wrote:
> On Thu, Nov 22, 2012 at 9:28 AM, <[email protected]> wrote:
> > From: Satheesh Rajendran <[email protected]>
> >
> >
> > Signed-off-by: Satheesh Rajendran <[email protected]>
> > ---
> > virttest/virsh.py | 197
> > +++++++++++++++++++++++++++++++++++++++++++++++++++++
> > 1 files changed, 197 insertions(+), 0 deletions(-)
> >
> > diff --git a/virttest/virsh.py b/virttest/virsh.py
> > index cab73af..bdaa596 100644
> > --- a/virttest/virsh.py
> > +++ b/virttest/virsh.py
> > @@ -1052,6 +1052,203 @@ def pool_create_as(name, pool_type, target,
> > extra="", **dargs):
> > logging.error("Failed to create pool: %s." % detail)
> > return False
> >
> > +def pool_list(option="", extra="", **dargs):
> > + """
> > + Prints the pool information of Host
> > +
> > + @param: option: options given to command
> > + --all - gives all pool details, including inactive
> > + --inactive - gives only inactive pool details
> > + --details - Gives the complete details about the pools
> > + @param: extra: to provide extra options(to enter invalid options)
> > + """
> > +
> > + cmd = "pool-list %s %s" % (option, extra)
> > + return command(cmd, **dargs)
> > +
> > +
> > +def pool_define_as(name, pool_type, target, extra="", **dargs):
> > + """
> > + Define the pool from the arguments
> > +
> > + @param: name: Name of the pool to be defined
> > + @param: typ: Type of the pool to be defined
> > + dir - file system directory
> > + disk - Physical Disk Device
> > + fs - Pre-formatted Block Device
> > + netfs - Network Exported Directory
> > + iscsi - iSCSI Target
> > + logical - LVM Volume Group
> > + mpath - Multipath Device Enumerater
> > + scsi - SCSI Host Adapter
> > + @param: target: libvirt uri to send guest to
> > + @param: extra: Free-form string of options
> > + @param: dargs: standardized virsh function API keywords
> > + @return: True if pool define command was successful
> > + """
> > +
> > + if not name:
> > + logging.error("Please give a pool name")
>
> ^ Only logging an error message seems too little. I'd either return
> False right away, or implement fault tolerance (automatically choose a
> name such as pool-[randomstring]).
Thanks for pointing out this.
The function should expect the pool name argument, I have not raised
TestFail here in these functions, in order to have negative cases
aswell, As you mentioned I should have return False just after the error
logging, would update it in the next version.
> > + types = [ 'dir', 'fs', 'netfs', 'disk', 'iscsi', 'logical' ]
> > +
> > + if pool_type and pool_type not in types:
> > + logging.error("Only support pool types: %s." % types)
> > + elif not pool_type:
> > + pool_type = types[0]
> > +
> > + logging.info("Define %s type pool %s" % (pool_type, name))
> > + cmd = "pool-define-as --name %s --type %s --target %s %s" \
> > + % (name, pool_type, target, extra)
>
> ^ Better to use implicit line continuation
>
> cmd = ("pool-define-as --name %s --type %s --target %s %s" %
> (name, pool_type, target, extra))
>
> > + dargs['ignore_status'] = False
> > + try:
> > + command(cmd, **dargs)
> > + return True
> > + except error.CmdError, detail:
> > + logging.error("Failed to define pool: %s." % detail)
> > + return False
> > +
> > +
> > +def pool_start(name, extra="", **dargs):
> > + """
> > + Start the defined pool
> > + @param: name: Name of the pool to be started
> > + @param: extra: Free-form string of options
> > + @param: dargs: standardized virsh function API keywords
> > + @return: True if pool start command was successful
> > + """
> > +
> > + if not name:
> > + logging.error("Please give a pool name")
> > +
> > + cmd = "pool-start %s %s" % (name, extra)
> > + dargs['ignore_status'] = False
> > + try:
> > + command(cmd, **dargs)
> > + return True
> > + except error.CmdError, detail:
> > + logging.error("Failed to start a pool: %s" % detail)
> > + return False
> > +
> > +
> > +def pool_autostart(name, extra="", **dargs):
> > + """
> > + Mark for autostart of a pool
>
> Mark a pool to auto start would be better...
>
> Is this the equivalent of 'starting the pool with host start'?
>
yes, it is the same functionality.
> > + @param: name: Name of the pool to be mark for autostart
> > + @param: extra: Free-form string of options
> > + @param: dargs: standardized virsh function API keywords
> > + @return: True if pool autostart command was successful
> > + """
> > +
> > + if not name:
> > + logging.error("Please give a pool name")
>
> Same comment as above, either return False or pick a name with a known
> prefix and random suffix.
>
Sure.
> > + cmd = "pool-autostart %s %s" % (name, extra)
> > + dargs['ignore_status'] = False
> > + try:
> > + command(cmd, **dargs)
> > + return True
> > + except error.CmdError, detail:
> > + logging.error("Failed to mark autostart to a pool" % detail)
> > + return False
> > +
> > +def pool_undefine(name, extra="", **dargs):
> > + """
> > + Undefine the given pool
> > +
> > + @param: name: Name of the pool to be undefined
> > + @param: extra: Free-form string of options
> > + @param: dargs: standardized virsh function API keywords
> > + @return: True if pool undefine command was successful
> > + """
> > +
> > + if not name:
> > + logging.error("Please give a pool name")
>
> Same as above.
>
Sure.
> > + cmd = "pool-undefine %s %s" % (name, extra)
> > + dargs['ignore_status'] = False
> > + try:
> > + command(cmd, **dargs)
> > + return True
> > + except error.CmdError, detail:
> > + logging.debug("Failed to undefine the pool" % detail)
> > + return False
> > +
> > +
> > +def vol_create_as(name, pool_name, capacity, allocation, format, \
>
> The backslash is unnecessary.
>
Would update it.
> > + extra="", **dargs):
> > + """
> > + To create the volumes on different available pool
> > +
> > + @param: name: Name of the volume to be created
> > + @param: pool_name: Name of the pool to be used
> > + @param: capacity: Size of the volume
> > + @param: allocaltion: Size of the volume to be pre-allocated
> > + @param: format: volume formats(e.g. raw, qed, qcow2)
> > + @param: extra: Free-form string of options
> > + @param: dargs: standardized virsh function API keywords
> > + @return: True if pool undefine command was successful
> > + """
> > +
> > + cmd = "vol-create-as"
> > + if not pool_name:
> > + logging.error("Please give a available pool name")
> > + else:
> > + cmd += " --pool %s" % (pool_name)
> > + if not name:
> > + logging.error("Please give a volume name")
>
> ^ Hmmm, I'm starting to think there's a reason for logging errors
> rather than making the functions return right away. And what would be
> the reason? Negative tests?
>
Yes, In general, I have not raised test fail for negative tests,
but have missed to return False just after failures, would update in the
version 2.
> > + else:
> > + cmd += " %s" % (name)
> > + if not capacity:
> > + logging.error("Please give the capacity of a volume")
> > + else:
> > + cmd += " --capacity %s" % (capacity)
> > + if allocation:
> > + cmd += " --allocation %s" % (allocation)
> > + if format:
> > + cmd += " --format %s" % (format)
> > + if extra:
> > + cmd += " %s" % (extra)
> > +
> > + try:
> > + command(cmd, **dargs)
> > + return True
> > + except error.CmdError, detail:
> > + logging.error("Failed to create a volume %s", detail)
> > + return False
> > +
> > +
> > +def vol_list(pool_name, extra="", **dargs):
> > + """
> > + List the volumes for a given pool
> > + """
> > +
> > + if not pool_name:
> > + logging.error("Please give a pool name")
> > + cmd = "vol-list %s %s" % (pool_name, extra)
> > + try:
> > + return command(cmd, **dargs)
> > + except error.CmdError, detail:
> > + logging.debug("Failed to list the volume\n %s", detail)
> > + return False
> > +
> > +def vol_delete(vol_name, pool_name, extra="", **dargs):
> > + """
> > + Delete a given volume
> > + """
> > +
> > + if not vol_name:
> > + logging.error("Please give a volume name to delete")
> > + if not pool_name:
> > + logging.error("Please give the respective pool name")
> > +
> > + cmd = "vol-delete %s %s %s" % (vol_name, pool_name, extra)
> > + try:
> > + command(cmd, **dargs)
> > + return True
> > + except CmdError, detail:
> > + logging.error("Failed to delete a volume %", detail)
> > + return False
> > +
> >
> > def capabilities(option='', **dargs):
> > """
> > --
> > 1.7.1
> >
Thanks Lucas, for the quick review, would send the updated version soon.
:)
> > _______________________________________________
> > Virt-test-devel mailing list
> > [email protected]
> > https://www.redhat.com/mailman/listinfo/virt-test-devel
>
>
>
_______________________________________________
Autotest-kernel mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/autotest-kernel
