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

Re: [libvirt] [PATCH v1 3/4] virsh: Expose virDomain{Get,Set}Time



On Thu, Feb 13, 2014 at 07:51:44PM +0100, Michal Privoznik wrote:
> These APIs are exposed under new virsh command 'domtime' which both gets
> and sets (not at the same time of course :)).
>
> Signed-off-by: Michal Privoznik <mprivozn redhat com>
> ---
>  tools/virsh-domain-monitor.c | 126 +++++++++++++++++++++++++++++++++++++++++++
>  tools/virsh.pod              |  16 ++++++
>  2 files changed, 142 insertions(+)
>
> diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
> index de4afbb..8e21e37 100644
> --- a/tools/virsh-domain-monitor.c
> +++ b/tools/virsh-domain-monitor.c
> @@ -1391,6 +1391,126 @@ cleanup:
>  }
>
>  /*
> + * "domtime" command
> + */
> +static const vshCmdInfo info_domtime[] = {
> +    {.name = "help",
> +     .data = N_("domain time")
> +    },
> +    {.name = "desc",
> +     .data = N_("Gets or sets a domain time")
> +    },
> +    {.name = NULL}
> +};
> +
> +static const vshCmdOptDef opts_domtime[] = {
> +    {.name = "domain",
> +     .type = VSH_OT_DATA,
> +     .flags = VSH_OFLAG_REQ,
> +     .help = N_("domain name, id or uuid")
> +    },
> +    {.name = "now",
> +     .type = VSH_OT_BOOL,
> +     .help = N_("set current host time")
> +    },
> +    {.name = "pretty",
> +     .type = VSH_OT_BOOL,
> +     .help = N_("print domain's time in human readable form")
> +    },
> +    {.name = "sync",
> +     .type = VSH_OT_BOOL,
> +     .help = N_("instead of setting given time, synchronize from domain's RTC"),
> +    },
> +    {.name = "time",
> +     .type = VSH_OT_INT,
> +     .help = N_("time to set")
> +    },
> +    {.name = NULL}
> +};
> +
> +static bool
> +cmdDomTime(vshControl *ctl, const vshCmd *cmd)
> +{
> +    virDomainPtr dom;
> +    bool ret = false;
> +    bool now = vshCommandOptBool(cmd, "now");
> +    bool pretty = vshCommandOptBool(cmd, "pretty");
> +    bool sync = vshCommandOptBool(cmd, "sync");
> +    bool doSet = false;
> +    long long guest_time;
> +    const char *timezone = NULL;
> +    int rv;
> +
> +    if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
> +        return false;
> +
> +    rv = vshCommandOptLongLong(cmd, "time", &guest_time);
> +
> +    if (rv < 0) {
> +        /* invalid integer format */

vshCommandOptLongLong() does not set an error, please set one.

> +        goto cleanup;
> +    } else if (rv > 0) {
> +        /* --time is used, so set time instead of get time.
> +         * However, --time and --now are mutually exclusive. */
> +        if (now) {
> +            vshError(ctl, _("--time and --now are mutually exclusive"));
> +            goto cleanup;
> +        }
> +
> +        /* Neither is --time and --sync */
> +        if (sync) {
> +            vshError(ctl, _("--time and --sync are mutually exclusive"));
> +            goto cleanup;
> +
> +        }
> +        doSet = true;
> +    }
> +
> +    if (sync && now) {
> +        vshError(ctl, _("--sync and --now are mutually exclusive"));
> +        goto cleanup;
> +    }
> +

And VSH_EXCLUSIVE_OPTIONS will deal with the rest for you (or it's
_EXPR variant if you already have the booleans in some variable.

> +    /* --now or --sync means setting */
> +    doSet |= now | sync;
> +
> +    if (doSet) {
> +        if (now && ((guest_time = time(NULL)) == (time_t) -1)) {
> +            vshError(ctl, _("unable to get current time"));
> +            goto cleanup;
> +        }
> +        if (virDomainSetTime(dom, guest_time, timezone,

You don't make the use of 'timezone' anywhere in the code.  And it has
the same problem as 'time' with older GCCs.

> +                             sync ? VIR_DOMAIN_TIME_SYNC : 0) < 0)
> +            goto cleanup;
> +    } else {
> +        if (virDomainGetTime(dom, &guest_time, 0) < 0)
> +            goto cleanup;
> +
> +        if (pretty) {
> +            char timestr[100];
> +            time_t cur_time = guest_time;
> +            struct tm time_info;
> +
> +            if (!gmtime_r(&cur_time, &time_info)) {
> +                vshError(ctl, _("Unable to format time"));
> +                goto cleanup;
> +            }
> +            strftime(timestr, sizeof(timestr), "%Y-%m-%d-%H:%M:%S", &time_info);

use space instead of dash (hyphen) after the date, better than that is
to use "%F" instead of "%Y-%m-%d" and even best would be to use "%c".

Question on the side, can you get the timezone from the guest agent,
too?  That would be great...

> +
> +            vshPrint(ctl, _("Time: %s"), timestr);
> +        } else {
> +            vshPrint(ctl, _("Time: %llu"), guest_time);
> +        }
> +    }
> +
> +    ret = true;
> +
> +cleanup:
> +    virDomainFree(dom);
> +    return ret;
> +}
> +
> +/*
>   * "list" command
>   */
>  static const vshCmdInfo info_list[] = {
> @@ -1946,6 +2066,12 @@ const vshCmdDef domMonitoringCmds[] = {
>       .info = info_domstate,
>       .flags = 0
>      },
> +    {.name = "domtime",
> +     .handler = cmdDomTime,
> +     .opts = opts_domtime,
> +     .info = info_domtime,
> +     .flags = 0
> +    },
>      {.name = "list",
>       .handler = cmdList,
>       .opts = opts_list,
> diff --git a/tools/virsh.pod b/tools/virsh.pod
> index f221475..40cb5b5 100644
> --- a/tools/virsh.pod
> +++ b/tools/virsh.pod
> @@ -969,6 +969,22 @@ Convert a domain Id (or UUID) to domain name
>  Returns state about a domain.  I<--reason> tells virsh to also print
>  reason for the state.
>
> +=item B<domtime> I<domain> { [I<--now>] [I<--pretty>] [I<--sync>]
> +[I<--time> B<time>] }
> +
> +Gets or sets the domain's system time. When run without any arguments
> +(but I<domain>), the current domain's system time is printed out. The

or '--pretty', I'd reword it to say '--now', '--time' and '--sync' set
the time in the guest, if none of them is specified, then the current
time is printed out.  It also clears out the rest of the paragraph.

> +I<--pretty> modifier can be used to print the time in more human
> +readable form. When I<--time> B<time> is specified, the domain's time is
> +not get but set instead. The I<--now> modifier acts like if it was an
> +alias for I<--time> B<$now>, which means it sets the time that is
> +currently on the host virsh is running at. In both cases (setting and
> +getting), time is in seconds relative to Epoch of 1970-01-01 in UTC.
> +The I<--sync> modifies the set behavior a bit: The time passed is
> +ignored, but the time to set is read from domain's RTC instead. Please
> +note, that some hypervisors may require a guest agent to be configured
> +in order to get or set the guest time.
> +
>  =item B<domcontrol> I<domain>
>
>  Returns state of an interface to VMM used to control a domain.  For
> --
> 1.8.5.3
>

Other than the mentioned nuances the patch looks fine.

Martin

Attachment: signature.asc
Description: Digital signature


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