On 03/23/2011 11:36 AM, Daniel P. Berrange wrote:
> The new commands vol-upload and vol-download, allow a local file
> to be transferred to/from a storage volume.
>
> * tools/virsh.c: Add vol-upload and vol-download commands
> ---
> tools/virsh.c | 230 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 files changed, 230 insertions(+), 0 deletions(-)
Shame on you for forgetting tools/virsh.pod (and on me for forgetting to
mention it on v1 and v2). However, documentation can be written in the
week between feature freeze and the actual 0.9.0 release, so while I'd
rather you write some up, I'm okay if you push this patch with only the
nits below fixed, and save the documentation for next week (and I'll
help write that, if necessary, since I forgot to mention it sooner), if
that ensures that we get this into 0.9.0.
> + if (vshCommandOptULongLong(cmd, "length", &length) < 0) {
> + vshError(ctl, _("Unable to parse integer"));
> + return FALSE;
> + }
> +
> + if (!(vol = vshCommandOptVol(ctl, cmd, "vol", "pool", &name))) {
> + return FALSE;
> + }
> +
> + if (vshCommandOptString(cmd, "file", &file) < 0)
> + goto cleanup;
Missing a call to vshError(ctl, _("file must not be empty"));
> +static int
> +cmdVolDownload (vshControl *ctl, const vshCmd *cmd)
> +{
> +
> + if (vshCommandOptString(cmd, "file", &file) < 0)
> + goto cleanup;
Again.
> +
> + if ((fd = open(file, O_WRONLY|O_TRUNC|O_CREAT|O_EXCL, 0666)) < 0) {
O_TRUNC should be omitted (when O_CREAT|O_EXCL is in effect, you are
guaranteeing that there is nothing to truncate; while Linux silently
ignores the O_TRUNC, someone else might conceivably fail with EINVAL on
the odd combination).
> + created = false;
> + if (errno != EEXIST ||
> + (fd = open(file, O_WRONLY|O_TRUNC|O_CREAT, 0666)) < 0) {
Here, the O_CREAT can be omitted (you just proved the file exited), but
that's not as essential as omitting the O_TRUNC on the previous open.
Reluctant ACK, with the above nits (minus virsh.pod) fixed.
--
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