[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 3/5] Add vol-create-upload, vol-upload and vol-download commands to virsh



On 03/15/2011 06:30 AM, Daniel P. Berrange wrote:
> * tools/virsh.c: Add vol-create-upload, vol-upload
>   and vol-download commands
> ---
>  .x-sc_avoid_write |    1 +
>  tools/virsh.c     |  225 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 226 insertions(+), 0 deletions(-)
> 
> diff --git a/.x-sc_avoid_write b/.x-sc_avoid_write
> index f6fc1b2..0784984 100644
> --- a/.x-sc_avoid_write
> +++ b/.x-sc_avoid_write
> @@ -5,5 +5,6 @@
>  ^src/util/util\.c$
>  ^src/xen/xend_internal\.c$
>  ^daemon/libvirtd.c$
> +^tools/virsh\.c$

Jim Meyering has a pending upstream patch for gnulib that will get rid
of the .x-sc* files in favor of cfg.mk; if that goes in first, it will
affect this patch.

Do we really need to add an unprotected write() to virsh.c?  That's such
a big file to exempt, and it would be nicer if we could just exempt a
helper file that does the single usage while keeping the overall virsh.c
file protected.

>  ^gnulib/
>  ^tools/console.c$
> diff --git a/tools/virsh.c b/tools/virsh.c
> index 42ebd55..9c4eac8 100644
> --- a/tools/virsh.c
> +++ b/tools/virsh.c
> @@ -263,6 +263,9 @@ static int vshCommandOptString(const vshCmd *cmd, const char *name,
>  static int vshCommandOptLongLong(const vshCmd *cmd, const char *name,
>                                   long long *value)
>      ATTRIBUTE_NONNULL(3) ATTRIBUTE_RETURN_CHECK;
> +static int vshCommandOptULongLong(const vshCmd *cmd, const char *name,
> +                                  unsigned long long *value)
> +    ATTRIBUTE_NONNULL(3) ATTRIBUTE_RETURN_CHECK;

Is it worth adding the helper API in a separate patch, to make it easier
to cherry pick just vshCommandOptULongLong and some followup patch that
uses it independently of vol-upload?

> +/*
> + * "vol-upload" command
> + */
> +static const vshCmdInfo info_vol_upload[] = {
> +    {"help", gettext_noop("upload a file into a volume")},

I thought we had a syntax-check rule that required you to use N_ instead
of gettext_noop here.  If we don't, that's probably worth adding, to
enforce consistency with the rest of the file.

> +    {"desc", gettext_noop("Upload a file into a volume")},
> +    {NULL, NULL}
> +};
> +
> +static const vshCmdOptDef opts_vol_upload[] = {
> +    {"pool", VSH_OT_STRING, 0, N_("pool name or uuid")},
> +    {"vol", VSH_OT_DATA, VSH_OFLAG_REQ, N_("vol name, key or path")},
> +    {"file", VSH_OT_DATA, VSH_OFLAG_REQ, gettext_noop("file")},

The above are required,

> +    {"offset", VSH_OT_INT, 0, N_("volume offset to upload to") },
> +    {"length", VSH_OT_INT, 0, N_("amount of data to upload") },

But these two should be optional (offset of 0, length of entire file
[and that in turn depends on answering my question in patch 2/5 about
whether 0 for length behaves special, or whether we need some other
sentinel like -1]).

> +    unsigned long long offset, length;
> +
> +    if (!vshConnectionUsability(ctl, ctl->conn))
> +        goto cleanup;
> +
> +    if (!(vol = vshCommandOptVol(ctl, cmd, "vol", "pool", &name))) {
> +        return FALSE;
> +    }
> +
> +    if (vshCommandOptULongLong(cmd, "offset", &offset) < 0)
> +        offset = 0;
> +
> +    if (vshCommandOptULongLong(cmd, "length", &length) < 0)
> +        length = 0;

Not quite the right usage pattern.  If the result is < 0, then that was
a syntax error (such as --offset=foo) and should be diagnosed.
Meanwhile, if the result is 0, then this uses offset uninitialized.

> +    if (virStreamSendAll(st, cmdVolUploadSource, &fd) < 0) {
> +        vshError(ctl, "cannot send data to volume %s", name);
> +        goto cleanup;
> +    }
> +
> +    if (close(fd) < 0) {

s/close/VIR_CLOSE/

> +        vshError(ctl, "cannot close file %s", file);
> +        virStreamAbort(st);
> +        goto cleanup;
> +    }
> +    fd = -1;

then you don't need this line.

> +    if (fd != -1)
> +        close(fd);

VIR_FORCE_CLOSE - 'make syntax-check' should have caught this

> +    return ret;
> +}
> +
> +
> +
> +/*
> + * "vol-download" command
> + */
> +static const vshCmdInfo info_vol_download[] = {
> +    {"help", gettext_noop("Download a volume to a file")},
> +    {"desc", gettext_noop("Download a volume to a file")},

N_()

> +    {NULL, NULL}
> +};
> +
> +static const vshCmdOptDef opts_vol_download[] = {
> +    {"pool", VSH_OT_STRING, 0, N_("pool name or uuid")},
> +    {"vol", VSH_OT_DATA, VSH_OFLAG_REQ, N_("vol name, key or path")},
> +    {"file", VSH_OT_DATA, VSH_OFLAG_REQ, gettext_noop("file")},
> +    {"offset", VSH_OT_INT, 0, N_("volume offset to download from") },
> +    {"length", VSH_OT_INT, 0, N_("amount of data to download") },
> +    {NULL, 0, 0, NULL}
> +};
> +
> +
> +static int
> +cmdVolDownloadSink(virStreamPtr st ATTRIBUTE_UNUSED,
> +                   const char *bytes, size_t nbytes, void *opaque)
> +{
> +    int *fd = opaque;
> +
> +    return write(*fd, bytes, nbytes);
> +}

Consistency - for upload, you put the helper before the
vshCmdInfo/vshCmdOptDef declarations, but for download, you put it in
between the declarations and their associated function.  I like the
style used for upload.

> +
> +    if (vshCommandOptULongLong(cmd, "offset", &offset) < 0)
> +        offset = 0;

Same usage problems as for upload.

> +
> +    if (vshCommandOptULongLong(cmd, "length", &length) < 0)
> +        length = 0;
> +
> +    if (vshCommandOptString(cmd, "file", &file) < 0)
> +        goto cleanup;
> +
> +    if ((fd = open(file, O_WRONLY|O_TRUNC|O_CREAT, 0600)) < 0) {

Should --mode be allowed as an optional argument?

> +
> +    if (close(fd) < 0) {

VIR_CLOSE

>  
> +
>  /*
>   * "net-edit" command
>   */
> @@ -10534,6 +10736,7 @@ static const vshCmdDef storageVolCmds[] = {
>      {"vol-create", cmdVolCreate, opts_vol_create, info_vol_create},
>      {"vol-create-from", cmdVolCreateFrom, opts_vol_create_from, info_vol_create_from},
>      {"vol-delete", cmdVolDelete, opts_vol_delete, info_vol_delete},
> +    {"vol-download", cmdVolDownload, opts_vol_download, info_vol_download },
>      {"vol-dumpxml", cmdVolDumpXML, opts_vol_dumpxml, info_vol_dumpxml},
>      {"vol-info", cmdVolInfo, opts_vol_info, info_vol_info},
>      {"vol-key", cmdVolKey, opts_vol_key, info_vol_key},
> @@ -10541,6 +10744,7 @@ static const vshCmdDef storageVolCmds[] = {
>      {"vol-name", cmdVolName, opts_vol_name, info_vol_name},
>      {"vol-path", cmdVolPath, opts_vol_path, info_vol_path},
>      {"vol-pool", cmdVolPool, opts_vol_pool, info_vol_pool},
> +    {"vol-upload", cmdVolUpload, opts_vol_upload, info_vol_upload },
>      {"vol-wipe", cmdVolWipe, opts_vol_wipe, info_vol_wipe},
>      {NULL, NULL, NULL, NULL}

No vol-create-upload command?  Is the commit message out-of-date, or is
this patch incomplete?

-- 
Eric Blake   eblake redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]