[libvirt] [PATCH] virsh: add --start option to the define command
I often find myself doing virsh "define blah.xml; start blah". I figured adding this would be a easier^Hlazier way to do it. --- tools/virsh-domain.c | 20 ++++++++++++++++++-- 1 files changed, 18 insertions(+), 2 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index e91939c..1a3af34 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -5597,6 +5597,7 @@ static const vshCmdInfo info_define[] = { static const vshCmdOptDef opts_define[] = { {"file", VSH_OT_DATA, VSH_OFLAG_REQ, N_("file containing an XML domain description")}, + {"start", VSH_OT_BOOL, 0, N_("start the domain after creation")}, {NULL, 0, 0, NULL} }; @@ -5607,6 +5608,7 @@ cmdDefine(vshControl *ctl, const vshCmd *cmd) const char *from = NULL; bool ret = true; char *buffer; + bool start; if (vshCommandOptString(cmd, "file", &from) <= 0) return false; @@ -5614,17 +5616,31 @@ cmdDefine(vshControl *ctl, const vshCmd *cmd) if (virFileReadAll(from, VSH_MAX_XML_FILE, &buffer) < 0) return false; + start = vshCommandOptBool(cmd, "start"); + dom = virDomainDefineXML(ctl->conn, buffer); VIR_FREE(buffer); if (dom != NULL) { vshPrint(ctl, _("Domain %s defined from %s\n"), virDomainGetName(dom), from); - virDomainFree(dom); } else { vshError(ctl, _("Failed to define domain from %s"), from); - ret = false; + return false; } + + /* Start the domain if the user requested it and it was defined */ + if (start) { + if (virDomainCreate(dom) < 0) { + vshError(ctl, _("Failed to start domain %s, which was " + "successfully defined."), virDomainGetName(dom)); + ret = false; + } else { + vshPrint(ctl, _("Domain %s started\n"), virDomainGetName(dom)); + } + } + + virDomainFree(dom); return ret; } -- 1.7.8.6
On 01/08/2013 02:36 PM, Doug Goldstein wrote:
I often find myself doing virsh "define blah.xml; start blah". I figured adding this would be a easier^Hlazier way to do it. --- tools/virsh-domain.c | 20 ++++++++++++++++++-- 1 files changed, 18 insertions(+), 2 deletions(-)
Offhand, I like it. However, We have virDomainDefineXML with no flags, but we have virDomainCreateXML with flags; maybe the better approach is to add a new creation flag that says that in addition to starting the domain, we also make it persistent at the same time. But if we do that, it would argue that 'virsh create --persistent blah.xml' is nicer than 'virsh define --start blah.xml', at least in that the former needs only 1 API call for new libvirt (but falls back to 2 API calls when talking to older libvirt), while the latter always needs 2 API calls. Or maybe it means we need to add virDomainDefineXMLFlags(). Anyone else want to throw some paint on the bikeshed on how best to make the user experience nicer? -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Tue, Jan 8, 2013 at 5:41 PM, Eric Blake <eblake@redhat.com> wrote:
On 01/08/2013 02:36 PM, Doug Goldstein wrote:
I often find myself doing virsh "define blah.xml; start blah". I figured adding this would be a easier^Hlazier way to do it. --- tools/virsh-domain.c | 20 ++++++++++++++++++-- 1 files changed, 18 insertions(+), 2 deletions(-)
Offhand, I like it. However,
We have virDomainDefineXML with no flags, but we have virDomainCreateXML with flags; maybe the better approach is to add a new creation flag that says that in addition to starting the domain, we also make it persistent at the same time.
But if we do that, it would argue that 'virsh create --persistent blah.xml' is nicer than 'virsh define --start blah.xml', at least in that the former needs only 1 API call for new libvirt (but falls back to 2 API calls when talking to older libvirt), while the latter always needs 2 API calls.
Or maybe it means we need to add virDomainDefineXMLFlags().
Anyone else want to throw some paint on the bikeshed on how best to make the user experience nicer?
I'm good with either. Honestly from an ease of use, virsh create --persistent might make more sense. Just trying to think of how I have to support libvirt use by the other engineers in the office. I'll resubmit the patch shortly. To go off on a tangent a bit, it feels like sometimes we're hindered with virsh by compatibility. Originally the interface was more aligned to demo the API of libvirt but now we're leaning towards more user friendly use. I've kicked around in my head hashing out a virsh 2.0 command line that would be more inline with end users and not necessarily libvirt API users. -- Doug Goldstein
On 01/09/2013 12:41 AM, Eric Blake wrote:
But if we do that, it would argue that 'virsh create --persistent blah.xml' is nicer than 'virsh define --start blah.xml', at least in that the former needs only 1 API call for new libvirt (but falls back to 2 API calls when talking to older libvirt), while the latter always needs 2 API calls.
virsh create --persistent is more natural to me especially w.r.t usability, i.e. virsh create --persistent --console blah.xml vs. virsh define --start --console blah.xml. (also consider --paused, --autodestroy). -- Mit freundlichen Grüßen/Kind Regards Viktor Mihajlovski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martina Köderitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On Tue, Jan 08, 2013 at 04:41:58PM -0700, Eric Blake wrote:
On 01/08/2013 02:36 PM, Doug Goldstein wrote:
I often find myself doing virsh "define blah.xml; start blah". I figured adding this would be a easier^Hlazier way to do it. --- tools/virsh-domain.c | 20 ++++++++++++++++++-- 1 files changed, 18 insertions(+), 2 deletions(-)
Offhand, I like it. However,
We have virDomainDefineXML with no flags, but we have virDomainCreateXML with flags; maybe the better approach is to add a new creation flag that says that in addition to starting the domain, we also make it persistent at the same time.
But if we do that, it would argue that 'virsh create --persistent blah.xml' is nicer than 'virsh define --start blah.xml', at least in that the former needs only 1 API call for new libvirt (but falls back to 2 API calls when talking to older libvirt), while the latter always needs 2 API calls.
Or maybe it means we need to add virDomainDefineXMLFlags().
Anyone else want to throw some paint on the bikeshed on how best to make the user experience nicer?
While I think the virsh idea is fine, I don't want to see this done at the virDomainDefine API level, since it just duplicates functionality already present. Just have virsh make an API call to the existing create API. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 01/09/2013 03:28 AM, Daniel P. Berrange wrote:
We have virDomainDefineXML with no flags, but we have virDomainCreateXML with flags; maybe the better approach is to add a new creation flag that says that in addition to starting the domain, we also make it persistent at the same time.
But if we do that, it would argue that 'virsh create --persistent blah.xml' is nicer than 'virsh define --start blah.xml', at least in that the former needs only 1 API call for new libvirt (but falls back to 2 API calls when talking to older libvirt), while the latter always needs 2 API calls.
Or maybe it means we need to add virDomainDefineXMLFlags().
Anyone else want to throw some paint on the bikeshed on how best to make the user experience nicer?
While I think the virsh idea is fine, I don't want to see this done at the virDomainDefine API level, since it just duplicates functionality already present. Just have virsh make an API call to the existing create API.
I agree that we don't really need to add virDomainDefineXMLFlags(). But I'm not quite clear on your stance on adding a new flag to the existing virDomainCreateXML(). A single API call is always nicer than two API calls for atomicity reasons. That is, I'm proposing that we add a new flag, and that: virDomainCreateXML(conn, xml, VIR_DOMAIN_CREATE_PERSISTENT) be used to both start and define a domain in one call, and only if that flag is unrecognized do we fall back to the two-API sequence of virDomainCreateXML(conn, xml, 0) && virDomainDefineXML(conn, xml) or the equivalent two-API sequence of dom=virDomainDefineXML(conn, xml) && virDomainCreate(dom) -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Wed, Jan 09, 2013 at 09:49:31AM -0700, Eric Blake wrote:
On 01/09/2013 03:28 AM, Daniel P. Berrange wrote:
We have virDomainDefineXML with no flags, but we have virDomainCreateXML with flags; maybe the better approach is to add a new creation flag that says that in addition to starting the domain, we also make it persistent at the same time.
But if we do that, it would argue that 'virsh create --persistent blah.xml' is nicer than 'virsh define --start blah.xml', at least in that the former needs only 1 API call for new libvirt (but falls back to 2 API calls when talking to older libvirt), while the latter always needs 2 API calls.
Or maybe it means we need to add virDomainDefineXMLFlags().
Anyone else want to throw some paint on the bikeshed on how best to make the user experience nicer?
While I think the virsh idea is fine, I don't want to see this done at the virDomainDefine API level, since it just duplicates functionality already present. Just have virsh make an API call to the existing create API.
I agree that we don't really need to add virDomainDefineXMLFlags(). But I'm not quite clear on your stance on adding a new flag to the existing virDomainCreateXML(). A single API call is always nicer than two API calls for atomicity reasons. That is, I'm proposing that we add a new flag, and that:
virDomainCreateXML(conn, xml, VIR_DOMAIN_CREATE_PERSISTENT)
I don't really like this either. I think it is good that the APIs are separate because it gives applications greater clarity on error handling. eg with this API you could get an case where the API defines the XML, starts the VM, fails and then tries to undefine the XML but fails that too. Now the caller sees an error, but can't know whether the XML was undefined or not. If the app does the separate API calls they have full control over what happens in that situation
be used to both start and define a domain in one call, and only if that flag is unrecognized do we fall back to the two-API sequence of
virDomainCreateXML(conn, xml, 0) && virDomainDefineXML(conn, xml)
or the equivalent two-API sequence of
dom=virDomainDefineXML(conn, xml) && virDomainCreate(dom)
Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Tue, Jan 08, 2013 at 04:41:58PM -0700, Eric Blake wrote:
On 01/08/2013 02:36 PM, Doug Goldstein wrote:
I often find myself doing virsh "define blah.xml; start blah". I figured adding this would be a easier^Hlazier way to do it. --- tools/virsh-domain.c | 20 ++++++++++++++++++-- 1 files changed, 18 insertions(+), 2 deletions(-)
Offhand, I like it. However,
We have virDomainDefineXML with no flags, but we have virDomainCreateXML with flags; maybe the better approach is to add a new creation flag that says that in addition to starting the domain, we also make it persistent at the same time.
But if we do that, it would argue that 'virsh create --persistent blah.xml' is nicer than 'virsh define --start blah.xml', at least in that the former needs only 1 API call for new libvirt (but falls back to 2 API calls when talking to older libvirt), while the latter always needs 2 API calls.
I like that since it would also remove some confusion about the diffference between create and define from an end user perspective. Cheers, -- Guido
Or maybe it means we need to add virDomainDefineXMLFlags().
Anyone else want to throw some paint on the bikeshed on how best to make the user experience nicer?
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Fri, Jan 11, 2013 at 2:51 PM, Guido Günther <agx@sigxcpu.org> wrote:
On Tue, Jan 08, 2013 at 04:41:58PM -0700, Eric Blake wrote:
On 01/08/2013 02:36 PM, Doug Goldstein wrote:
I often find myself doing virsh "define blah.xml; start blah". I figured adding this would be a easier^Hlazier way to do it. --- tools/virsh-domain.c | 20 ++++++++++++++++++-- 1 files changed, 18 insertions(+), 2 deletions(-)
Offhand, I like it. However,
We have virDomainDefineXML with no flags, but we have virDomainCreateXML with flags; maybe the better approach is to add a new creation flag that says that in addition to starting the domain, we also make it persistent at the same time.
But if we do that, it would argue that 'virsh create --persistent blah.xml' is nicer than 'virsh define --start blah.xml', at least in that the former needs only 1 API call for new libvirt (but falls back to 2 API calls when talking to older libvirt), while the latter always needs 2 API calls.
I like that since it would also remove some confusion about the diffference between create and define from an end user perspective. Cheers, -- Guido
I'd like to still add "virsh define --start" in the end. Possibly when --start is passed, virsh would actually call virDomainCreateXML(). My feeling is that virsh started out really demoing the API but its really a fundamental user tool now and we should strive to make it friendly to use. -- Doug Goldstein
On 02/18/13 06:11, Doug Goldstein wrote:
On Fri, Jan 11, 2013 at 2:51 PM, Guido Günther <agx@sigxcpu.org> wrote:
On Tue, Jan 08, 2013 at 04:41:58PM -0700, Eric Blake wrote:
On 01/08/2013 02:36 PM, Doug Goldstein wrote:
I often find myself doing virsh "define blah.xml; start blah". I figured adding this would be a easier^Hlazier way to do it. --- tools/virsh-domain.c | 20 ++++++++++++++++++-- 1 files changed, 18 insertions(+), 2 deletions(-)
I'd like to still add "virsh define --start" in the end. Possibly when --start is passed, virsh would actually call virDomainCreateXML(). My feeling is that virsh started out really demoing the API but its really a fundamental user tool now and we should strive to make it friendly to use.
I think that the original version of your patch is usable although it's lacking documentation in the man page. If you mind reposting it with that added it might be a intermediate road until somebody will be motivated enough to add the flag for virDomainCreateXML. Peter
On Thu, Feb 21, 2013 at 9:55 AM, Peter Krempa <pkrempa@redhat.com> wrote:
On 02/18/13 06:11, Doug Goldstein wrote:
On Fri, Jan 11, 2013 at 2:51 PM, Guido Günther <agx@sigxcpu.org> wrote:
On Tue, Jan 08, 2013 at 04:41:58PM -0700, Eric Blake wrote:
On 01/08/2013 02:36 PM, Doug Goldstein wrote:
I often find myself doing virsh "define blah.xml; start blah". I figured adding this would be a easier^Hlazier way to do it. --- tools/virsh-domain.c | 20 ++++++++++++++++++-- 1 files changed, 18 insertions(+), 2 deletions(-)
I'd like to still add "virsh define --start" in the end. Possibly when --start is passed, virsh would actually call virDomainCreateXML(). My feeling is that virsh started out really demoing the API but its really a fundamental user tool now and we should strive to make it friendly to use.
I think that the original version of your patch is usable although it's lacking documentation in the man page. If you mind reposting it with that added it might be a intermediate road until somebody will be motivated enough to add the flag for virDomainCreateXML.
Peter
Locally I've implemented VIR_DOMAIN_START_PERSISTENT for virDomainCreateXML() and the qemu driver. I'm tweaking virsh to call that when you do virsh define --start instead of virDomainDefine(). It still needs some massaging and docs fix ups locally before I'm ready to push it for review. But basically there's: virsh create --persistent domain.xml virsh define --start domain.xml I'll let you guys decide what parts you want to keep and what parts you don't. -- Doug Goldstein
participants (7)
-
Daniel P. Berrange -
Doug Goldstein -
Doug Goldstein -
Eric Blake -
Guido Günther -
Peter Krempa -
Viktor Mihajlovski