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

Re: [libvirt] [PATCH v5 10/10] virsh: Add iothreadadd and iothreaddel commands



[Peter seems to have inadvertently only replied to me on this one and
then I blindly reply-all'd to him - took me a few to realize this - I'll
just attach the response I sent back so that we keep everything on-list
as well...]

With any luck the response follows!

--- Begin Message ---

On 04/27/2015 11:06 AM, Peter Krempa wrote:
> On Fri, Apr 24, 2015 at 12:06:02 -0400, John Ferlan wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1161617
>>
>> Add command to allow adding and removing IOThreads from the domain including
>> the configuration and live domain.
>>
>> $ virsh iothreadadd --help
>>   NAME
>>     iothreadadd - add an IOThread to the guest domain
>>
>>   SYNOPSIS
>>     iothreadadd <domain> <id> [--config] [--live] [--current]
>>
>>   DESCRIPTION
>>     Add an IOThread to the guest domain.
>>
>>   OPTIONS
>>     [--domain] <string>  domain name, id or uuid
>>     [--id] <number>  iothread for the new IOThread
>>     --config         affect next boot
>>     --live           affect running domain
>>     --current        affect current domain
>>
>> $ virsh iothreaddel --help
>>   NAME
>>     iothreaddel - delete an IOThread from the guest domain
>>
>>   SYNOPSIS
>>     iothreaddel <domain> <id> [--config] [--live] [--current]
>>
>>   DESCRIPTION
>>     Delete an IOThread from the guest domain.
>>
>>   OPTIONS
>>     [--domain] <string>  domain name, id or uuid
>>     [--id] <number>  iothread_id for the IOThread to delete
>>     --config         affect next boot
>>     --live           affect running domain
>>     --current        affect current domain
>>
>> Assuming a running $dom with multiple IOThreads assigned and that
>> that the $dom has disks assigned to IOThread 1 and IOThread 2:
>>
>> $ virsh iothreadinfo $dom
>>  IOThread ID     CPU Affinity
>>  ---------------------------------------------------
>>   1               2
>>   2               3
>>   3               0-1
>>
>> $ virsh iothreadadd $dom 1
>> error: invalid argument: an IOThread is already using iothread_id '1' in iothreadpids
>>
>> $ virsh iothreadadd $dom 1 --config
>> error: invalid argument: an IOThread is already using iothread_id '1' in persistent iothreadids
>>
>> $ virsh iothreadadd $dom 4
>> $ virsh iothreadinfo $dom
>>  IOThread ID     CPU Affinity
>>  ---------------------------------------------------
>>   1               2
>>   2               3
>>   3               0-1
>>   4               0-3
>>
>> $ virsh iothreadinfo $dom --config
>>  IOThread ID     CPU Affinity
>>  ---------------------------------------------------
>>   1               2
>>   2               3
>>   3               0-1
>>
>> $ virsh iothreadadd $dom 4 --config
>> $ virsh iothreadinfo $dom --config
>>  IOThread ID     CPU Affinity
>>   ---------------------------------------------------
>>     1               2
>>     2               3
>>     3               0-1
>>     4               0-3
>>
>> Assuming the same original configuration
>>
>> $ virsh iothreaddel $dom 1
>> error: invalid argument: cannot remove IOThread 1 since it is being used by disk path '/home/vm-images/f18'
>>
>> $ virsh iothreaddel $dom 3
>>
>> $ virsh iothreadinfo $dom
>>  IOThread ID     CPU Affinity
>>  ---------------------------------------------------
>>   1               2
>>   2               3
>>
>> $ virsh iothreadinfo $dom --config
>>  IOThread ID     CPU Affinity
>>  ---------------------------------------------------
>>   1               2
>>   2               3
>>   3               0-1
>>
>> Signed-off-by: John Ferlan <jferlan redhat com>
>> ---
>>  tools/virsh-domain.c | 164 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>  tools/virsh.pod      |  31 ++++++++++
>>  2 files changed, 195 insertions(+)
>>
>> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
>> index a190050..513f6df 100644
>> --- a/tools/virsh-domain.c
>> +++ b/tools/virsh-domain.c
>> @@ -6896,6 +6896,158 @@ cmdIOThreadPin(vshControl *ctl, const vshCmd *cmd)
>>  }
>>  
>>  /*
>> + * "iothreadadd" command
>> + */
>> +static const vshCmdInfo info_iothreadadd[] = {
>> +    {.name = "help",
>> +     .data = N_("add an IOThread to the guest domain")
>> +    },
>> +    {.name = "desc",
>> +     .data = N_("Add an IOThread to the guest domain.")
>> +    },
>> +    {.name = NULL}
>> +};
>> +
>> +static const vshCmdOptDef opts_iothreadadd[] = {
>> +    {.name = "domain",
>> +     .type = VSH_OT_DATA,
>> +     .flags = VSH_OFLAG_REQ,
>> +     .help = N_("domain name, id or uuid")
>> +    },
>> +    {.name = "id",
>> +     .type = VSH_OT_INT,
>> +     .flags = VSH_OFLAG_REQ,
>> +     .help = N_("iothread for the new IOThread")
>> +    },
>> +    {.name = "config",
>> +     .type = VSH_OT_BOOL,
>> +     .help = N_("affect next boot")
>> +    },
>> +    {.name = "live",
>> +     .type = VSH_OT_BOOL,
>> +     .help = N_("affect running domain")
>> +    },
>> +    {.name = "current",
>> +     .type = VSH_OT_BOOL,
>> +     .help = N_("affect current domain")
>> +    },
>> +    {.name = NULL}
>> +};
>> +
>> +static bool
>> +cmdIOThreadAdd(vshControl *ctl, const vshCmd *cmd)
>> +{
>> +    virDomainPtr dom;
>> +    int iothread_id = 0;
>> +    bool ret = false;
>> +    bool config = vshCommandOptBool(cmd, "config");
>> +    bool live = vshCommandOptBool(cmd, "live");
>> +    bool current = vshCommandOptBool(cmd, "current");
>> +    unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT;
>> +
>> +    VSH_EXCLUSIVE_OPTIONS_VAR(current, live);
>> +    VSH_EXCLUSIVE_OPTIONS_VAR(current, config);
>> +
>> +    if (config)
>> +        flags |= VIR_DOMAIN_AFFECT_CONFIG;
>> +    if (live)
>> +        flags |= VIR_DOMAIN_AFFECT_LIVE;
>> +
>> +    if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
>> +        return false;
>> +
>> +    if (vshCommandOptInt(cmd, "id", &iothread_id) < 0 || iothread_id <= 0) {
>> +        vshError(ctl, _("Invalid IOThread id value: '%d'"), iothread_id);
> 
> In case you provide something that is not a number the error message is
> suboptimal:
> 
> $ virsh iothreadadd test asdf
> error: Invalid IOThread id value: '0'
> 
>> +        goto cleanup;
>> +    }
>> +
>> +    if (virDomainAddIOThread(dom, iothread_id, flags) < 0)
>> +        goto cleanup;
>> +
>> +    ret = true;
>> +
>> + cleanup:
>> +    virDomainFree(dom);
>> +    return ret;
>> +}
>> +
>> +/*
>> + * "iothreaddel" command
>> + */
>> +static const vshCmdInfo info_iothreaddel[] = {
>> +    {.name = "help",
>> +     .data = N_("delete an IOThread from the guest domain")
>> +    },
>> +    {.name = "desc",
>> +     .data = N_("Delete an IOThread from the guest domain.")
>> +    },
>> +    {.name = NULL}
>> +};
>> +
>> +static const vshCmdOptDef opts_iothreaddel[] = {
>> +    {.name = "domain",
>> +     .type = VSH_OT_DATA,
>> +     .flags = VSH_OFLAG_REQ,
>> +     .help = N_("domain name, id or uuid")
>> +    },
>> +    {.name = "id",
>> +     .type = VSH_OT_INT,
>> +     .flags = VSH_OFLAG_REQ,
>> +     .help = N_("iothread_id for the IOThread to delete")
>> +    },
>> +    {.name = "config",
>> +     .type = VSH_OT_BOOL,
>> +     .help = N_("affect next boot")
>> +    },
>> +    {.name = "live",
>> +     .type = VSH_OT_BOOL,
>> +     .help = N_("affect running domain")
>> +    },
>> +    {.name = "current",
>> +     .type = VSH_OT_BOOL,
>> +     .help = N_("affect current domain")
>> +    },
>> +    {.name = NULL}
>> +};
>> +
>> +static bool
>> +cmdIOThreadDel(vshControl *ctl, const vshCmd *cmd)
>> +{
>> +    virDomainPtr dom;
>> +    int iothread_id = 0;
>> +    bool ret = false;
>> +    bool config = vshCommandOptBool(cmd, "config");
>> +    bool live = vshCommandOptBool(cmd, "live");
>> +    bool current = vshCommandOptBool(cmd, "current");
>> +    unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT;
>> +
>> +    VSH_EXCLUSIVE_OPTIONS_VAR(current, live);
>> +    VSH_EXCLUSIVE_OPTIONS_VAR(current, config);
>> +
>> +    if (config)
>> +        flags |= VIR_DOMAIN_AFFECT_CONFIG;
>> +    if (live)
>> +        flags |= VIR_DOMAIN_AFFECT_LIVE;
>> +
>> +    if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
>> +        return false;
>> +
>> +    if (vshCommandOptInt(cmd, "id", &iothread_id) < 0 || iothread_id <= 0) {
>> +        vshError(ctl, _("Invalid IOThread id value: '%d'"), iothread_id);
> 
> Same here.
> 
>> +        goto cleanup;
>> +    }
>> +
>> +    if (virDomainDelIOThread(dom, iothread_id, flags) < 0)
>> +        goto cleanup;
>> +
>> +    ret = true;
>> +
>> + cleanup:
>> +    virDomainFree(dom);
>> +    return ret;
>> +}
>> +
>> +/*
>>   * "cpu-compare" command
>>   */
>>  static const vshCmdInfo info_cpu_compare[] = {
>> @@ -12845,6 +12997,18 @@ const vshCmdDef domManagementCmds[] = {
>>       .info = info_iothreadpin,
>>       .flags = 0
>>      },
>> +    {.name = "iothreadadd",
>> +     .handler = cmdIOThreadAdd,
>> +     .opts = opts_iothreadadd,
>> +     .info = info_iothreadadd,
>> +     .flags = 0
>> +    },
>> +    {.name = "iothreaddel",
>> +     .handler = cmdIOThreadDel,
>> +     .opts = opts_iothreaddel,
>> +     .info = info_iothreaddel,
>> +     .flags = 0
>> +    },
>>      {.name = "send-key",
>>       .handler = cmdSendKey,
>>       .opts = opts_send_key,
>> diff --git a/tools/virsh.pod b/tools/virsh.pod
>> index d642a69..091abc6 100644
>> --- a/tools/virsh.pod
>> +++ b/tools/virsh.pod
>> @@ -1416,6 +1416,37 @@ If no flag is specified, behavior is different depending on hypervisor.
>>  B<Note>: The expression is sequentially evaluated, so "0-15,^8" is
>>  identical to "9-14,0-7,15" but not identical to "^8,0-15".
>>  
>> +=item B<iothreadadd> I<domain> I<iothread_id>
>> +[[I<--config>] [I<--live>] | [I<--current>]]
>> +
>> +Add a new IOThread to the domain using the specified I<iothread_id>.
>> +If the I<iothread_id> already exists, the command will fail. The
>> +I<iothread_id> must be greater than zero.
>> +
>> +If I<--live> is specified, affect a running guest. If the guest is not
>> +running an error is returned.
>> +If I<--config> is specified, affect the next boot of a persistent guest.
>> +If I<--current> is specified or I<--live> and I<--config> are not specified,
>> +affect the current guest state.
>> +Both I<--live> and I<--config> flags may be given, but if not flag is
>> +specified, behavior is different depending on hypervisor.
> 
> The last two sentences conflict semantically. One says that if you use
> --current (or not specify, --live && --config) then the current state is
> modified. The second one says that it might be arbitrary. Delete the
> last one.
> 

These must've been remnants from when more flags/options were present and I was just moving code around and removing things...  Certainly doesn't make sense!


>> +
>> +=item B<iothreaddel> I<domain> I<iothread_id>
>> +[[I<--config>] [I<--live>] | [I<--current>]]
>> +
>> +Delete an IOThread from the domain using the specified I<iothread_id>.
>> +If an IOThread is currently assigned to a disk resource such as via the
>> +B<attach-disk> command, then the attempt to remove the IOThread will fail.
>> +If the I<iothread_id> does not exist an error will occur.
>> +
>> +If I<--live> is specified, affect a running guest. If the guest is not
>> +running an error is returned.
>> +If I<--config> is specified, affect the next boot of a persistent guest.
>> +If I<--current> is specified or I<--live> and I<--config> are not specified,
>> +affect the current guest state.
>> +Both I<--live> and I<--config> flags may be given, but if not flag is
>> +specified, behavior is different depending on hypervisor.
> 
> Same comment as above.
> 
>> +
>>  =item B<managedsave> I<domain> [I<--bypass-cache>]
>>  [{I<--running> | I<--paused>}] [I<--verbose>]
> 
> ACK with the man page spelling fixed and error reporting improved.
> 
> Peter
> 

The following will be squashed in. Thank you for your time and detailed
efforts on this series!

John

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 513f6df..4b627e1 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -6956,7 +6956,11 @@ cmdIOThreadAdd(vshControl *ctl, const vshCmd *cmd)
     if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
         return false;
 
-    if (vshCommandOptInt(cmd, "id", &iothread_id) < 0 || iothread_id <= 0) {
+    if (vshCommandOptInt(cmd, "id", &iothread_id) < 0) {
+        vshError(ctl, "%s", _("Unable to parse integer parameter"));
+        goto cleanup;
+    }
+    if (iothread_id <= 0) {
         vshError(ctl, _("Invalid IOThread id value: '%d'"), iothread_id);
         goto cleanup;
     }
@@ -7032,7 +7036,11 @@ cmdIOThreadDel(vshControl *ctl, const vshCmd *cmd)
     if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
         return false;
 
-    if (vshCommandOptInt(cmd, "id", &iothread_id) < 0 || iothread_id <= 0) {
+    if (vshCommandOptInt(cmd, "id", &iothread_id) < 0) {
+        vshError(ctl, "%s", _("Unable to parse integer parameter"));
+        goto cleanup;
+    }
+    if (iothread_id <= 0) {
         vshError(ctl, _("Invalid IOThread id value: '%d'"), iothread_id);
         goto cleanup;
     }
diff --git a/tools/virsh.pod b/tools/virsh.pod
index 091abc6..f8496f3 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -1428,8 +1428,6 @@ running an error is returned.
 If I<--config> is specified, affect the next boot of a persistent guest.
 If I<--current> is specified or I<--live> and I<--config> are not specified,
 affect the current guest state.
-Both I<--live> and I<--config> flags may be given, but if not flag is
-specified, behavior is different depending on hypervisor.
 
 =item B<iothreaddel> I<domain> I<iothread_id>
 [[I<--config>] [I<--live>] | [I<--current>]]
@@ -1444,8 +1442,6 @@ running an error is returned.
 If I<--config> is specified, affect the next boot of a persistent guest.
 If I<--current> is specified or I<--live> and I<--config> are not specified,
 affect the current guest state.
-Both I<--live> and I<--config> flags may be given, but if not flag is
-specified, behavior is different depending on hypervisor.
 
 =item B<managedsave> I<domain> [I<--bypass-cache>]
 [{I<--running> | I<--paused>}] [I<--verbose>]

--- End Message ---

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