[libvirt] [PATCH v2] screenshot: Expose the new API in virsh

* tools/virsh.c: Add screenshot command * tools/virsh.pod: Document new command * src/libvirt.c: Fix off-be-one error --- diff to v1: - make filename optional and generate filename when missing src/libvirt.c | 2 +- tools/virsh.c | 152 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ tools/virsh.pod | 9 +++ 3 files changed, 162 insertions(+), 1 deletions(-) diff --git a/src/libvirt.c b/src/libvirt.c index 5a5439d..cfb9e3b 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -2461,7 +2461,7 @@ error: * The screen ID is the sequential number of screen. In case of multiple * graphics cards, heads are enumerated before devices, e.g. having * two graphics cards, both with four heads, screen ID 5 addresses - * the first head on the second card. + * the second head on the second card. * * Returns a string representing the mime-type of the image format, or * NULL upon error. The caller must free() the returned value. diff --git a/tools/virsh.c b/tools/virsh.c index e35637d..aad930e 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -1879,6 +1879,157 @@ cmdDump(vshControl *ctl, const vshCmd *cmd) return ret; } +static const vshCmdInfo info_screenshot[] = { + {"help", N_("take a screenshot of a current domain console and store it " + "into a file")}, + {"desc", N_("screenshot of a current domain console")}, + {NULL, NULL} +}; + +static const vshCmdOptDef opts_screenshot[] = { + {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, + {"file", VSH_OT_DATA, VSH_OFLAG_NONE, N_("where to store the screenshot")}, + {"screen", VSH_OT_INT, VSH_OFLAG_NONE, N_("ID of a screen to take screenshot of")}, + {NULL, 0, 0, NULL} +}; + +static int cmdScreenshotSink(virStreamPtr st ATTRIBUTE_UNUSED, + const char *bytes, size_t nbytes, void *opaque) +{ + int *fd = opaque; + + return safewrite(*fd, bytes, nbytes); +} + +/** + * Generate string: '<domain name>-<timestamp>>[<extension>]' + */ +static char * +vshGenerFileName(vshControl *ctl, virDomainPtr dom) { + char timestr[100]; + struct timeval cur_time; + struct tm time_info; + const char *ext = NULL; + const char *hypType = NULL; + char *ret = NULL; + + /* We should be already connected, but doesn't + * hurt to check */ + if (!vshConnectionUsability(ctl, ctl->conn)) + return NULL; + + if (!dom) { + vshError(ctl, "%s", _("Invalid domain supplied")); + return NULL; + } + + if (!(hypType = virConnectGetType(ctl->conn))) { + vshError(ctl, "%s", _("Can't query hypervisor's type")); + return NULL; + } + + if (STREQ(hypType, "QEMU")) + ext = ".ppm"; + else if (STREQ(hypType, "VBOX")) + ext = ".png"; + /* add hypervisors here */ + + gettimeofday(&cur_time, NULL); + localtime_r(&cur_time.tv_sec, &time_info); + strftime(timestr, sizeof(timestr), "%Y-%m-%d-%H:%M:%S", &time_info); + + if (virAsprintf(&ret, "%s-%s%s", virDomainGetName(dom), + timestr, ext ? ext : "") < 0) { + vshError(ctl, "%s", _("Out of memory")); + return false; + } + + return ret; +} + +static bool +cmdScreenshot(vshControl *ctl, const vshCmd *cmd) { + virDomainPtr dom; + const char *name = NULL; + char *file = NULL; + int fd = -1; + virStreamPtr st = NULL; + unsigned int screen = 0; + unsigned int flags = 0; /* currently unused */ + int ret = false; + bool created = true; + bool generated = false; + char *mime = NULL; + + if (!vshConnectionUsability(ctl, ctl->conn)) + return false; + + if (vshCommandOptString(cmd, "file", (const char **) &file) < 0) { + vshError(ctl, "%s", _("file must not be empty")); + return false; + } + + if (vshCommandOptInt(cmd, "screen", (int*) &screen) < 0) { + vshError(ctl, "%s", _("invalid screen ID")); + return false; + } + + if (!(dom = vshCommandOptDomain(ctl, cmd, &name))) + return false; + + if (!file) { + if (!(file=vshGenerFileName(ctl, dom))) + return false; + generated = true; + } + + if ((fd = open(file, O_WRONLY|O_CREAT|O_EXCL, 0666)) < 0) { + created = false; + if (errno != EEXIST || + (fd = open(file, O_WRONLY|O_TRUNC, 0666)) < 0) { + vshError(ctl, _("cannot create file %s"), file); + goto cleanup; + } + } + + st = virStreamNew(ctl->conn, 0); + + mime = virDomainScreenshot(dom, st, screen, flags); + if (mime == NULL) { + vshError(ctl, _("could not take a screenshot of %s"), name); + goto cleanup; + } + + if (virStreamRecvAll(st, cmdScreenshotSink, &fd) < 0) { + vshError(ctl, _("could not receive data from domain %s"), name); + goto cleanup; + } + + if (VIR_CLOSE(fd) < 0) { + vshError(ctl, _("cannot close file %s"), file); + goto cleanup; + } + + if (virStreamFinish(st) < 0) { + vshError(ctl, _("cannot close stream on domain %s"), name); + goto cleanup; + } + + vshPrint(ctl, _("Screenshot saved to %s it's type is %s"), file, mime); + ret = true; + +cleanup: + if (ret == false && created) + unlink(file); + if (generated) + VIR_FREE(file); + virDomainFree(dom); + if (st) + virStreamFree(st); + VIR_FORCE_CLOSE(fd); + return ret; +} + /* * "resume" command */ @@ -10750,6 +10901,7 @@ static const vshCmdDef domManagementCmds[] = { {"resume", cmdResume, opts_resume, info_resume}, {"save", cmdSave, opts_save, info_save}, {"schedinfo", cmdSchedinfo, opts_schedinfo, info_schedinfo}, + {"screenshot", cmdScreenshot, opts_screenshot, info_screenshot}, {"setmaxmem", cmdSetmaxmem, opts_setmaxmem, info_setmaxmem}, {"setmem", cmdSetmem, opts_setmem, info_setmem}, {"setvcpus", cmdSetvcpus, opts_setvcpus, info_setvcpus}, diff --git a/tools/virsh.pod b/tools/virsh.pod index d11a0e3..5390f19 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -589,6 +589,15 @@ Therefore, -1 is a useful shorthand for 262144. B<Note>: The weight and cap parameters are defined only for the XEN_CREDIT scheduler and are now I<DEPRECATED>. +=item B<screenshot> I<domain-id> I<imagefilepath> optional I<--screen> B<screenID> + +Takes a screenshot of a current domain console and stores it into a file. +Optionally, if hypervisor supports more displays for a domain, I<screenID> +allows to specify from which should be the screenshot taken. It is the +sequential number of screen. In case of multiple graphics cards, heads +are enumerated before devices, e.g. having two graphics cards, both with +four heads, screen ID 5 addresses the second head on the second card. + =item B<setmem> I<domain-id> B<kilobytes> optional I<--config> I<--live> I<--current> -- 1.7.5.rc3

On 05/16/2011 10:57 AM, Michal Privoznik wrote:
* tools/virsh.c: Add screenshot command * tools/virsh.pod: Document new command * src/libvirt.c: Fix off-be-one error --- diff to v1: - make filename optional and generate filename when missing
+ +static int cmdScreenshotSink(virStreamPtr st ATTRIBUTE_UNUSED, + const char *bytes, size_t nbytes, void *opaque) +{ + int *fd = opaque; + + return safewrite(*fd, bytes, nbytes); +}
Hmm, this is identical to cmdVolDownloadSink. We should consolidate those into a single function; perhaps naming it vshStreamSink But that can be a separate followup patch.
+ +/** + * Generate string: '<domain name>-<timestamp>>[<extension>]'
s/>>/>/
+ */ +static char * +vshGenerFileName(vshControl *ctl, virDomainPtr dom) {
I'm used to seeing either Gen or Generate, not Gener. Also, for functions, we stick the first { on its own line.
+ + if (STREQ(hypType, "QEMU")) + ext = ".ppm"; + else if (STREQ(hypType, "VBOX")) + ext = ".png"; + /* add hypervisors here */
This seems fragile, especially if QEMU learns how to do more than one MIME type. Rather, you need to make the generator function take as input the MIME type output by virDomainSnapshot, and convert that to a file name.
+ + gettimeofday(&cur_time, NULL); + localtime_r(&cur_time.tv_sec, &time_info); + strftime(timestr, sizeof(timestr), "%Y-%m-%d-%H:%M:%S", &time_info); + + if (virAsprintf(&ret, "%s-%s%s", virDomainGetName(dom), + timestr, ext ? ext : "") < 0) { + vshError(ctl, "%s", _("Out of memory")); + return false;
return NULL; (this function returns a pointer, not a bool)
+ } + + return ret; +} + +static bool +cmdScreenshot(vshControl *ctl, const vshCmd *cmd) { + virDomainPtr dom; + const char *name = NULL; + char *file = NULL; + int fd = -1; + virStreamPtr st = NULL; + unsigned int screen = 0; + unsigned int flags = 0; /* currently unused */ + int ret = false; + bool created = true; + bool generated = false; + char *mime = NULL; + + if (!vshConnectionUsability(ctl, ctl->conn)) + return false; + + if (vshCommandOptString(cmd, "file", (const char **) &file) < 0) { + vshError(ctl, "%s", _("file must not be empty")); + return false; + } + + if (vshCommandOptInt(cmd, "screen", (int*) &screen) < 0) {
The cast looks spurious. Either we need to add vshCommandOptUInt, or you should just use 'int screen' instead of 'unsigned int screen', as well as check that screen >= 0.
+ vshError(ctl, "%s", _("invalid screen ID")); + return false; + } + + if (!(dom = vshCommandOptDomain(ctl, cmd, &name))) + return false; + + if (!file) { + if (!(file=vshGenerFileName(ctl, dom))) + return false; + generated = true; + } + + if ((fd = open(file, O_WRONLY|O_CREAT|O_EXCL, 0666)) < 0) { + created = false; + if (errno != EEXIST || + (fd = open(file, O_WRONLY|O_TRUNC, 0666)) < 0) { + vshError(ctl, _("cannot create file %s"), file); + goto cleanup; + } + }
I would delay opening fd until...
+ + st = virStreamNew(ctl->conn, 0); + + mime = virDomainScreenshot(dom, st, screen, flags); + if (mime == NULL) { + vshError(ctl, _("could not take a screenshot of %s"), name); + goto cleanup; + }
...after you know the mime type, so that the generation of a default file name knows what suffix to stick on the file by querying the mime type.
+ + if (virStreamRecvAll(st, cmdScreenshotSink, &fd) < 0) { + vshError(ctl, _("could not receive data from domain %s"), name); + goto cleanup; + } + + if (VIR_CLOSE(fd) < 0) { + vshError(ctl, _("cannot close file %s"), file); + goto cleanup; + } + + if (virStreamFinish(st) < 0) { + vshError(ctl, _("cannot close stream on domain %s"), name); + goto cleanup; + } + + vshPrint(ctl, _("Screenshot saved to %s it's type is %s"), file, mime);
Grammar: "Screenshot saved to %s, with type of %s"
+ ret = true; + +cleanup: + if (ret == false && created)
'ret == false' is discouraged by coding styles; use '!ret' instead.
+ unlink(file); + if (generated) + VIR_FREE(file); + virDomainFree(dom); + if (st) + virStreamFree(st); + VIR_FORCE_CLOSE(fd); + return ret; +} + /* * "resume" command */ @@ -10750,6 +10901,7 @@ static const vshCmdDef domManagementCmds[] = { {"resume", cmdResume, opts_resume, info_resume}, {"save", cmdSave, opts_save, info_save}, {"schedinfo", cmdSchedinfo, opts_schedinfo, info_schedinfo}, + {"screenshot", cmdScreenshot, opts_screenshot, info_screenshot},
This will need to be rebased to the latest.
{"setmaxmem", cmdSetmaxmem, opts_setmaxmem, info_setmaxmem}, {"setmem", cmdSetmem, opts_setmem, info_setmem}, {"setvcpus", cmdSetvcpus, opts_setvcpus, info_setvcpus}, diff --git a/tools/virsh.pod b/tools/virsh.pod index d11a0e3..5390f19 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -589,6 +589,15 @@ Therefore, -1 is a useful shorthand for 262144. B<Note>: The weight and cap parameters are defined only for the XEN_CREDIT scheduler and are now I<DEPRECATED>.
+=item B<screenshot> I<domain-id> I<imagefilepath> optional I<--screen> B<screenID>
Now that imagefilepath is also optional, I'd write this as: item B<screenshot> I<domain-id> optional I<imagefilepath> I<--screen> B<screenID>
+ +Takes a screenshot of a current domain console and stores it into a file. +Optionally, if hypervisor supports more displays for a domain, I<screenID> +allows to specify from which should be the screenshot taken. It is the
s/from which should be the screenshot taken/which screen will be captured/
+sequential number of screen. In case of multiple graphics cards, heads +are enumerated before devices, e.g. having two graphics cards, both with +four heads, screen ID 5 addresses the second head on the second card. + =item B<setmem> I<domain-id> B<kilobytes> optional I<--config> I<--live> I<--current>
We'll need to review a v3, but since you proposed this before freeze, and since it is exposing in virsh an API that was added for 0.9.2, I would be okay with pushing a v3 before the 0.9.2 release (that is, I'm arguing that adding a new API without corresponding virsh support represents a bug worth fixing, rather than this being a new virsh feature after the feature freeze). -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake
-
Michal Privoznik