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@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list