Re: [libvirt] [PATCH 4/4] virsh: Expose new virDomainFSTrim API

On 11/20/12 19:47, Michal Privoznik wrote: >under domfstrim command. Since API doesn't support all >parameters (some must be NULL or zero), don't expose >these in virsh neither. >--- > tools/virsh-domain.c | 40 ++++++++++++++++++++++++++++++++++++++++ > tools/virsh.pod | 12 ++++++++++++ > 2 files changed, 52 insertions(+), 0 deletions(-) > >diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c >index cc47383..4ea08f4 100644 >--- a/tools/virsh-domain.c >+++ b/tools/virsh-domain.c >@@ -8300,6 +8300,45 @@ cleanup: > return ret; > } > >+static const vshCmdInfo info_domfstrim[] = { >+ {"help", N_("Invoke fstrim on domain's mounted filesystems.")}, >+ {"desc", N_("Invoke fstrim on domain's mounted filesystems.")}, >+ {NULL, NULL} >+}; >+ >+static const vshCmdOptDef opts_domfstrim[] = { >+ {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, >+ {"minimum", VSH_OT_INT, 0, N_("Just a hint to ignore contiguous " >+ "free ranges smaller than this (Bytes)")}, Wouldn't it make sense to expose the mount point argument right now? I know it's not implemented yet, but when somebody does implement that we will need to change this later. >+ {NULL, 0, 0, NULL} >+}; >+static bool >+cmdDomFSTrim(vshControl *ctl, const vshCmd *cmd) >+{ >+ virDomainPtr dom = NULL; >+ bool ret = false; >+ unsigned long long minimum = 0; >+ unsigned int flags = 0; >+ >+ if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) >+ goto cleanup; >+ >+ if (vshCommandOptULongLong(cmd, "minimum", &minimum) < 0) { >+ vshError(ctl, _("Unable to parse integer parameter")); As you know what's the name of the parameter print it in the error message so the user is not left guessing where the error is. >+ goto cleanup; >+ } >+ >+ if (virDomainFSTrim(dom, NULL, minimum, flags) < 0) { >+ vshError(ctl, _("Unable to invoke fstrim")); >+ goto cleanup; >+ } >+ >+ ret = true; >+ >+cleanup: >+ return ret; >+} >+ > const vshCmdDef domManagementCmds[] = { > {"attach-device", cmdAttachDevice, opts_attach_device, > info_attach_device, 0}, >@@ -8332,6 +8371,7 @@ const vshCmdDef domManagementCmds[] = { > {"detach-interface", cmdDetachInterface, opts_detach_interface, > info_detach_interface, 0}, > {"domdisplay", cmdDomDisplay, opts_domdisplay, info_domdisplay, 0}, >+ {"domfstrim", cmdDomFSTrim, opts_domfstrim, info_domfstrim, 0}, > {"domhostname", cmdDomHostname, opts_domhostname, info_domhostname, 0}, > {"domid", cmdDomid, opts_domid, info_domid, 0}, > {"domif-setlink", cmdDomIfSetLink, opts_domif_setlink, info_domif_setlink, 0}, >diff --git a/tools/virsh.pod b/tools/virsh.pod >index 29be39e..c3fdcb7 100644 >--- a/tools/virsh.pod >+++ b/tools/virsh.pod >@@ -878,6 +878,18 @@ Output a URI which can be used to connect to the graphical display of the > domain via VNC, SPICE or RDP. If I<--include-password> is specified, the > SPICE channel password will be included in the URI. > >+=item B<domfstrim> I<domain> [I<--minimum> B<bytes>] >+ >+Issue a fstrim command on all mounted filesystems within a running >+domain. It discards blocks which are not in use by the filesystem. >+If I<--minimum> B<bytes> is specified, it tells guest kernel length >+of contiguous free range. Smaller than this may be ignored (this is >+a hint and the guest may not respect it). By increasing this value, >+the fstrim operation will complete more quickly for filesystems >+with badly fragmented free space, although not all blocks will >+be discarded. The default value is zero, meaning "discard >+every free block". >+ > =item B<domhostname> I<domain> > > Returns the hostname of a domain, if the hypervisor makes it available. > I'd rather see this with the ability to specify mountpoint to the api even if it still isn't supported.