[libvirt PATCH 0/5] Formalize the deprecation of arguments in virsh
virsh has several arguments that are better not used. This series introduces a formal way of marking them as deprecated. Tim Wiederhake (5): vsh: Set default log level to "warning" vsh: Introduce flag for deprecated options vsh: setmem: Mark '--kilobytes' as deprecated vsh: setmaxmem: Mark '--kilobytes' as deprecated vsh: secret-set-value: Mark '--base64' as deprecated tools/virsh-domain.c | 2 ++ tools/virsh-secret.c | 3 +-- tools/vsh.c | 11 +++++++++-- tools/vsh.h | 11 ++++++----- 4 files changed, 18 insertions(+), 9 deletions(-) -- 2.26.2
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- tools/vsh.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/vsh.h b/tools/vsh.h index 39f70913fe..e83e0eca9e 100644 --- a/tools/vsh.h +++ b/tools/vsh.h @@ -60,7 +60,7 @@ typedef enum { VSH_ERR_ERROR } vshErrorLevel; -#define VSH_DEBUG_DEFAULT VSH_ERR_ERROR +#define VSH_DEBUG_DEFAULT VSH_ERR_WARNING /* * virsh command line grammar: -- 2.26.2
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- tools/vsh.c | 11 +++++++++-- tools/vsh.h | 9 +++++---- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/tools/vsh.c b/tools/vsh.c index 8fb033249e..8b58db8896 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -321,7 +321,8 @@ vshCmddefCheckInternals(vshControl *ctl, char *name = (char *)opt->help; /* cast away const */ char *p; - if (opt->flags || !opt->help) { + if ((opt->flags != 0 && opt->flags != VSH_OFLAG_DEPRECATED) || + !opt->help) { vshError(ctl, _("parameter '%s' of command '%s' has incorrect alias option"), opt->name, cmd->name); return -1; /* alias options are tracked by the original name */ @@ -442,6 +443,11 @@ vshCmddefGetOption(vshControl *ctl, const vshCmdDef *cmd, const char *name, const vshCmdOptDef *opt = &cmd->opts[i]; if (STREQ(opt->name, name)) { + if (report && (opt->flags & VSH_OFLAG_DEPRECATED)) { + vshDebug(ctl, VSH_ERR_WARNING, + _("Warning: Option '%s' is deprecated\n"), name); + } + if (opt->type == VSH_OT_ALIAS) { char *value; @@ -697,7 +703,8 @@ vshCmddefHelp(const vshCmdDef *def) continue; } - fprintf(stdout, " %-15s %s\n", buf, _(opt->help)); + fprintf(stdout, " %-15s %s%s\n", buf, _(opt->help), + opt->flags & VSH_OFLAG_DEPRECATED ? " (deprecated)" : ""); } } fputc('\n', stdout); diff --git a/tools/vsh.h b/tools/vsh.h index e83e0eca9e..e85206b726 100644 --- a/tools/vsh.h +++ b/tools/vsh.h @@ -98,10 +98,11 @@ typedef enum { * Command Option Flags */ enum { - VSH_OFLAG_NONE = 0, /* without flags */ - VSH_OFLAG_REQ = (1 << 0), /* option required */ - VSH_OFLAG_EMPTY_OK = (1 << 1), /* empty string option allowed */ - VSH_OFLAG_REQ_OPT = (1 << 2), /* --optionname required */ + VSH_OFLAG_NONE = 0, /* without flags */ + VSH_OFLAG_REQ = (1 << 0), /* option required */ + VSH_OFLAG_EMPTY_OK = (1 << 1), /* empty string option allowed */ + VSH_OFLAG_REQ_OPT = (1 << 2), /* --optionname required */ + VSH_OFLAG_DEPRECATED = (1 << 3), /* option is deprecated */ }; /* forward declarations */ -- 2.26.2
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- tools/virsh-domain.c | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 7db88f700a..32bd048d26 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -8986,6 +8986,7 @@ static const vshCmdOptDef opts_setmem[] = { VIRSH_COMMON_OPT_DOMAIN_FULL(0), {.name = "kilobytes", .type = VSH_OT_ALIAS, + .flags = VSH_OFLAG_DEPRECATED, .help = "size" }, {.name = "size", -- 2.26.2
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- tools/virsh-domain.c | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 32bd048d26..9f0e3e1a5e 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -9065,6 +9065,7 @@ static const vshCmdOptDef opts_setmaxmem[] = { VIRSH_COMMON_OPT_DOMAIN_FULL(0), {.name = "kilobytes", .type = VSH_OT_ALIAS, + .flags = VSH_OFLAG_DEPRECATED, .help = "size" }, {.name = "size", -- 2.26.2
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- tools/virsh-secret.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tools/virsh-secret.c b/tools/virsh-secret.c index 7ca3e864da..5a1c4d7bf6 100644 --- a/tools/virsh-secret.c +++ b/tools/virsh-secret.c @@ -192,6 +192,7 @@ static const vshCmdOptDef opts_secret_set_value[] = { }, {.name = "base64", .type = VSH_OT_STRING, + .flags = VSH_OFLAG_DEPRECATED, .help = N_("base64-encoded secret value") }, {.name = NULL} @@ -225,8 +226,6 @@ cmdSecretSetValue(vshControl *ctl, const vshCmd *cmd) return false; if (base64) { - /* warn users that the --base64 option passed from command line is wrong */ - vshError(ctl, _("Passing secret value as command-line argument is insecure!")); secret_val = g_strdup(base64); secret_len = strlen(secret_val); } else if (filename) { -- 2.26.2
On 3/22/21 5:09 PM, Tim Wiederhake wrote:
virsh has several arguments that are better not used. This series introduces a formal way of marking them as deprecated.
Commit messages are rather sparse. What we currently have is hiding options we deem obsolete from users and replacing them with better ones (just :Ggrep VSH_OT_ALIAS). No error message, no warning. What makes these you picked special? I'm not against reporting that an option is obsolete, but I don't quite understand why we need a different way for obsoleting those three. I think plain warning reported in vshCmddefGetOption() would be just fine, wouldn't it? I don't think that ERROR level is justified for this - if I run `virsh -q` from a script I don't want to be bothered. Michal
On Tue, Mar 23, 2021 at 02:36:19PM +0100, Michal Privoznik wrote:
On 3/22/21 5:09 PM, Tim Wiederhake wrote:
virsh has several arguments that are better not used. This series introduces a formal way of marking them as deprecated.
Commit messages are rather sparse. What we currently have is hiding options we deem obsolete from users and replacing them with better ones (just :Ggrep VSH_OT_ALIAS). No error message, no warning. What makes these you picked special? I'm not against reporting that an option is obsolete, but I don't quite understand why we need a different way for obsoleting those three.
Also the general idea of deprecation is that the thing will be deleted eventually, which is not something we intended to do with these options. Basically there's a better way to do these things, but we're not going to break existing usage, so if users are happy with what they're doing they don't need to change. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 3/23/21 2:42 PM, Daniel P. Berrangé wrote:
On Tue, Mar 23, 2021 at 02:36:19PM +0100, Michal Privoznik wrote:
On 3/22/21 5:09 PM, Tim Wiederhake wrote:
virsh has several arguments that are better not used. This series introduces a formal way of marking them as deprecated.
Commit messages are rather sparse. What we currently have is hiding options we deem obsolete from users and replacing them with better ones (just :Ggrep VSH_OT_ALIAS). No error message, no warning. What makes these you picked special? I'm not against reporting that an option is obsolete, but I don't quite understand why we need a different way for obsoleting those three.
Also the general idea of deprecation is that the thing will be deleted eventually, which is not something we intended to do with these options. Basically there's a better way to do these things, but we're not going to break existing usage, so if users are happy with what they're doing they don't need to change.
To be fair we never promised virsh to be stable, did we? We are trying to keep it as backwards compatible as we can (and so far I guess we didn't have a single instance of bad example), but I wouldn't mind telling users (esp. in interactive mode) that --optionX is now called --optionY. Michal
On Tue, Mar 23, 2021 at 02:50:09PM +0100, Michal Privoznik wrote:
On 3/23/21 2:42 PM, Daniel P. Berrangé wrote:
On Tue, Mar 23, 2021 at 02:36:19PM +0100, Michal Privoznik wrote:
On 3/22/21 5:09 PM, Tim Wiederhake wrote:
virsh has several arguments that are better not used. This series introduces a formal way of marking them as deprecated.
Commit messages are rather sparse. What we currently have is hiding options we deem obsolete from users and replacing them with better ones (just :Ggrep VSH_OT_ALIAS). No error message, no warning. What makes these you picked special? I'm not against reporting that an option is obsolete, but I don't quite understand why we need a different way for obsoleting those three.
Also the general idea of deprecation is that the thing will be deleted eventually, which is not something we intended to do with these options. Basically there's a better way to do these things, but we're not going to break existing usage, so if users are happy with what they're doing they don't need to change.
To be fair we never promised virsh to be stable, did we? We are trying to keep it as backwards compatible as we can (and so far I guess we didn't have a single instance of bad example), but I wouldn't mind telling users (esp. in interactive mode) that --optionX is now called --optionY.
https://libvirt.org/support.html#virsh Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Tue, Mar 23, 2021 at 14:50:09 +0100, Michal Privoznik wrote:
On 3/23/21 2:42 PM, Daniel P. Berrangé wrote:
On Tue, Mar 23, 2021 at 02:36:19PM +0100, Michal Privoznik wrote:
On 3/22/21 5:09 PM, Tim Wiederhake wrote:
virsh has several arguments that are better not used. This series introduces a formal way of marking them as deprecated.
Commit messages are rather sparse. What we currently have is hiding options we deem obsolete from users and replacing them with better ones (just :Ggrep VSH_OT_ALIAS). No error message, no warning. What makes these you picked special? I'm not against reporting that an option is obsolete, but I don't quite understand why we need a different way for obsoleting those three.
Also the general idea of deprecation is that the thing will be deleted eventually, which is not something we intended to do with these options. Basically there's a better way to do these things, but we're not going to break existing usage, so if users are happy with what they're doing they don't need to change.
To be fair we never promised virsh to be stable, did we? We are trying to
I'd say we do, at least for the reasonably machine-usable interfaces (for output [1]), thus any input options should be treated as such.
keep it as backwards compatible as we can (and so far I guess we didn't have a single instance of bad example), but I wouldn't mind telling users (esp. in interactive mode) that --optionX is now called --optionY.
Well, that's what we do with the alias and documentation changes. But removing --optionX completely would be wrong: - It needlessly breaks scripts - If you decide to print a deprecation warning, the code usually won't be much simpler. - most cases are covered by use of _ALIAS to a better name The only thing that IMO should be removed but I didn't for compatibility is the 'secret-set-value's 'base64' parameter as that is insecure. There isn't a compatible replacement though. [1] Table outputs and other clearly human-targetted output may obviously break, but for many of the output cases we provide machine-friendly variants, such as '--uuid'/'--name' for listing APIs.
On 3/23/21 3:04 PM, Peter Krempa wrote:
On Tue, Mar 23, 2021 at 14:50:09 +0100, Michal Privoznik wrote:
On 3/23/21 2:42 PM, Daniel P. Berrangé wrote:
On Tue, Mar 23, 2021 at 02:36:19PM +0100, Michal Privoznik wrote:
On 3/22/21 5:09 PM, Tim Wiederhake wrote:
virsh has several arguments that are better not used. This series introduces a formal way of marking them as deprecated.
Commit messages are rather sparse. What we currently have is hiding options we deem obsolete from users and replacing them with better ones (just :Ggrep VSH_OT_ALIAS). No error message, no warning. What makes these you picked special? I'm not against reporting that an option is obsolete, but I don't quite understand why we need a different way for obsoleting those three.
Also the general idea of deprecation is that the thing will be deleted eventually, which is not something we intended to do with these options. Basically there's a better way to do these things, but we're not going to break existing usage, so if users are happy with what they're doing they don't need to change.
To be fair we never promised virsh to be stable, did we? We are trying to
I'd say we do, at least for the reasonably machine-usable interfaces (for output [1]), thus any input options should be treated as such.
keep it as backwards compatible as we can (and so far I guess we didn't have a single instance of bad example), but I wouldn't mind telling users (esp. in interactive mode) that --optionX is now called --optionY.
Well, that's what we do with the alias and documentation changes. But removing --optionX completely would be wrong:
Just to be clear, I am not advocating for removing anything.
- It needlessly breaks scripts - If you decide to print a deprecation warning, the code usually won't be much simpler. - most cases are covered by use of _ALIAS to a better name
The only thing that IMO should be removed but I didn't for compatibility is the 'secret-set-value's 'base64' parameter as that is insecure. There isn't a compatible replacement though.
That's debatable. Its not much worse than reading from a file. I mean, who has access to my $HISTFILE? Only me and root and in both cases the secret can be changed or read from the file (if the file is not deleted right away, and even then it could be recovered). Many tools accept passwords in clear text on cmd line (e.g. curl, wget). If anything, we could document why --base64 is dangerous (if we haven't done so yet). Michal
On Tue, Mar 23, 2021 at 15:19:44 +0100, Michal Privoznik wrote:
On 3/23/21 3:04 PM, Peter Krempa wrote:
On Tue, Mar 23, 2021 at 14:50:09 +0100, Michal Privoznik wrote:
On 3/23/21 2:42 PM, Daniel P. Berrangé wrote:
On Tue, Mar 23, 2021 at 02:36:19PM +0100, Michal Privoznik wrote:
[...]
The only thing that IMO should be removed but I didn't for compatibility is the 'secret-set-value's 'base64' parameter as that is insecure. There isn't a compatible replacement though.
That's debatable. Its not much worse than reading from a file. I mean, who has access to my $HISTFILE? Only me and root and in both cases the secret
It's not about HISTFILE, but about the process listing. On a default linux box, all users can list all other user's processes. If your password is an argument for a command, it will be readable for other users without the access to your directory. Arguably, the lifetime of virsh is very short, so it's extremely unlikely for anyone to notice, but it's insecure regardless.
can be changed or read from the file (if the file is not deleted right away, and even then it could be recovered). Many tools accept passwords in clear text on cmd line (e.g. curl, wget). If anything, we could document why
You should avoid use of those arguments if you are on a multi-user box.
--base64 is dangerous (if we haven't done so yet).
It is documented as such and also prints a warning as pointed out in the other reply.
On Tue, 2021-03-23 at 15:28 +0100, Peter Krempa wrote:
On Tue, Mar 23, 2021 at 15:19:44 +0100, Michal Privoznik wrote:
On 3/23/21 3:04 PM, Peter Krempa wrote:
On Tue, Mar 23, 2021 at 14:50:09 +0100, Michal Privoznik wrote:
On 3/23/21 2:42 PM, Daniel P. Berrangé wrote:
On Tue, Mar 23, 2021 at 02:36:19PM +0100, Michal Privoznik wrote:
[...]
The only thing that IMO should be removed but I didn't for compatibility is the 'secret-set-value's 'base64' parameter as that is insecure. There isn't a compatible replacement though.
That's debatable. Its not much worse than reading from a file. I mean, who has access to my $HISTFILE? Only me and root and in both cases the secret
It's not about HISTFILE, but about the process listing. On a default linux box, all users can list all other user's processes. If your password is an argument for a command, it will be readable for other users without the access to your directory.
Arguably, the lifetime of virsh is very short, so it's extremely unlikely for anyone to notice, but it's insecure regardless.
can be changed or read from the file (if the file is not deleted right away, and even then it could be recovered). Many tools accept passwords in clear text on cmd line (e.g. curl, wget). If anything, we could document why
You should avoid use of those arguments if you are on a multi-user box.
--base64 is dangerous (if we haven't done so yet).
It is documented as such and also prints a warning as pointed out in the other reply.
Hi all, thank you for your feedback! My motivation for starting this patch series was the desire to change the behavior of the virsh commands "create", "define", "snapshot- create", "cpu-compare", and "hypervisor-cpu-compare": Currently, those commands accept an "--validate" argument that enables RNG schema validation for the provided XML. In my opinion, this should be the default behavior. This series was supposed to lay the ground work, a formal deprecation system for commands and arguments. I understand that suddenly rejecting invalid XML where it was accepted previously and seemingly did the right thing breaks backwards compatibility, even if the only affected users are those with invalid XML which should be fixed anyway. To that end, I wrote a small set of patches that introduce a "--no-validate" argument to these commands, mark that flag as deprecated, and only then make the switch for enabling validation by default. For users with valid XML, this change is invisible. Users with invalid XML are being made aware that their XML is broken, but given the opportunity to continue operations by specifying "--no-validate" until their XML is fixed. Once a certain amount of time has passed, e.g. "two major releases" or "all officially supported distributions carry libvirt versions that have XML validation on by default", the deprecated "--no-validate" flags can be deleted. But alas, I was unaware of https://libvirt.org/support.html#virsh, (thanks Daniel!) I did not find this when grepping for "deprecat(ed|ion)": "Existing commands and arguments will not be removed or renamed". This guarantee prevents any change of this series or the unpublished one described above. ("Infinite" stability guarantees place an undue maintenance burden on libvirt as it prevents developers from fixing / removing outdated or insecure commands and arguments, in my opinion; but that is besides the point here.) I will abandon this series and retry with a version that simply outputs a warning if the user provides XML data without also using " --validate", similar to the one for "--base64". Cheers, Tim
On Tue, Mar 23, 2021 at 05:23:38PM +0100, Tim Wiederhake wrote:
On Tue, 2021-03-23 at 15:28 +0100, Peter Krempa wrote:
On Tue, Mar 23, 2021 at 15:19:44 +0100, Michal Privoznik wrote:
On 3/23/21 3:04 PM, Peter Krempa wrote:
On Tue, Mar 23, 2021 at 14:50:09 +0100, Michal Privoznik wrote:
On 3/23/21 2:42 PM, Daniel P. Berrangé wrote:
On Tue, Mar 23, 2021 at 02:36:19PM +0100, Michal Privoznik wrote:
[...]
The only thing that IMO should be removed but I didn't for compatibility is the 'secret-set-value's 'base64' parameter as that is insecure. There isn't a compatible replacement though.
That's debatable. Its not much worse than reading from a file. I mean, who has access to my $HISTFILE? Only me and root and in both cases the secret
It's not about HISTFILE, but about the process listing. On a default linux box, all users can list all other user's processes. If your password is an argument for a command, it will be readable for other users without the access to your directory.
Arguably, the lifetime of virsh is very short, so it's extremely unlikely for anyone to notice, but it's insecure regardless.
can be changed or read from the file (if the file is not deleted right away, and even then it could be recovered). Many tools accept passwords in clear text on cmd line (e.g. curl, wget). If anything, we could document why
You should avoid use of those arguments if you are on a multi-user box.
--base64 is dangerous (if we haven't done so yet).
It is documented as such and also prints a warning as pointed out in the other reply.
Hi all,
thank you for your feedback!
My motivation for starting this patch series was the desire to change the behavior of the virsh commands "create", "define", "snapshot- create", "cpu-compare", and "hypervisor-cpu-compare": Currently, those commands accept an "--validate" argument that enables RNG schema validation for the provided XML. In my opinion, this should be the default behavior. This series was supposed to lay the ground work, a formal deprecation system for commands and arguments.
I understand that suddenly rejecting invalid XML where it was accepted previously and seemingly did the right thing breaks backwards compatibility, even if the only affected users are those with invalid XML which should be fixed anyway. To that end, I wrote a small set of patches that introduce a "--no-validate" argument to these commands, mark that flag as deprecated, and only then make the switch for enabling validation by default.
For users with valid XML, this change is invisible. Users with invalid XML are being made aware that their XML is broken, but given the opportunity to continue operations by specifying "--no-validate" until their XML is fixed.
Their XML is not neccessarily broken. They may simply have included some XML elements from newer libvirt that are not relevant on the current libvirt and be fine with them being ignored. Yes it would be better if they actually tailored the XML for the specific libvirt, but the kind of people using virsh often don't care about this level of perfection. Note, that we automatically use validation with the 'virsh *edit' commands because those are for interactive users and they often make mistakes resulting in their changes silently disappearing. So in that case validating by default was a clear win with no real downside (assuming bug free schemas).
I will abandon this series and retry with a version that simply outputs a warning if the user provides XML data without also using " --validate", similar to the one for "--base64".
This will result in warnings for almost everyone who uses virsh, so I would not be in favour of that. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Tue, 2021-03-23 at 16:55 +0000, Daniel P. Berrangé wrote:
On Tue, Mar 23, 2021 at 05:23:38PM +0100, Tim Wiederhake wrote:
On Tue, 2021-03-23 at 15:28 +0100, Peter Krempa wrote:
On Tue, Mar 23, 2021 at 15:19:44 +0100, Michal Privoznik wrote:
On 3/23/21 3:04 PM, Peter Krempa wrote:
On Tue, Mar 23, 2021 at 14:50:09 +0100, Michal Privoznik wrote:
On 3/23/21 2:42 PM, Daniel P. Berrangé wrote: > On Tue, Mar 23, 2021 at 02:36:19PM +0100, Michal > Privoznik > wrote:
[...]
The only thing that IMO should be removed but I didn't for compatibility is the 'secret-set-value's 'base64' parameter as that is insecure. There isn't a compatible replacement though.
That's debatable. Its not much worse than reading from a file. I mean, who has access to my $HISTFILE? Only me and root and in both cases the secret
It's not about HISTFILE, but about the process listing. On a default linux box, all users can list all other user's processes. If your password is an argument for a command, it will be readable for other users without the access to your directory.
Arguably, the lifetime of virsh is very short, so it's extremely unlikely for anyone to notice, but it's insecure regardless.
can be changed or read from the file (if the file is not deleted right away, and even then it could be recovered). Many tools accept passwords in clear text on cmd line (e.g. curl, wget). If anything, we could document why
You should avoid use of those arguments if you are on a multi- user box.
--base64 is dangerous (if we haven't done so yet).
It is documented as such and also prints a warning as pointed out in the other reply.
Hi all,
thank you for your feedback!
My motivation for starting this patch series was the desire to change the behavior of the virsh commands "create", "define", "snapshot- create", "cpu-compare", and "hypervisor-cpu-compare": Currently, those commands accept an "--validate" argument that enables RNG schema validation for the provided XML. In my opinion, this should be the default behavior. This series was supposed to lay the ground work, a formal deprecation system for commands and arguments.
I understand that suddenly rejecting invalid XML where it was accepted previously and seemingly did the right thing breaks backwards compatibility, even if the only affected users are those with invalid XML which should be fixed anyway. To that end, I wrote a small set of patches that introduce a "--no-validate" argument to these commands, mark that flag as deprecated, and only then make the switch for enabling validation by default.
For users with valid XML, this change is invisible. Users with invalid XML are being made aware that their XML is broken, but given the opportunity to continue operations by specifying "--no-validate" until their XML is fixed.
Their XML is not neccessarily broken. They may simply have included some XML elements from newer libvirt that are not relevant on the current libvirt and be fine with them being ignored. Yes it would be better if they actually tailored the XML for the specific libvirt, but the kind of people using virsh often don't care about this level of perfection.
Good point, did not think about that possibility. Thanks!
Note, that we automatically use validation with the 'virsh *edit' commands because those are for interactive users and they often make mistakes resulting in their changes silently disappearing. So in that case validating by default was a clear win with no real downside (assuming bug free schemas).
I will abandon this series and retry with a version that simply outputs a warning if the user provides XML data without also using " --validate", similar to the one for "--base64".
This will result in warnings for almost everyone who uses virsh, so I would not be in favour of that.
I realized that too, once I hit "send". Let me see if something like "always validate; '--validate' => error out, no '--validate' => warn and continue" is feasible. Cheers, Tim
On Tue, Mar 23, 2021 at 14:36:19 +0100, Michal Privoznik wrote:
On 3/22/21 5:09 PM, Tim Wiederhake wrote:
virsh has several arguments that are better not used. This series introduces a formal way of marking them as deprecated.
Commit messages are rather sparse. What we currently have is hiding options we deem obsolete from users and replacing them with better ones (just :Ggrep VSH_OT_ALIAS). No error message, no warning. What makes these you picked special? I'm not against reporting that an option is obsolete, but I don't quite understand why we need a different way for obsoleting those three.
There's one exception seen in 5/5 and that is the 'base64' parameter of 'secret-set-value'. (It's not a boolean enabling base64 mode but rather a base64 value of the secret passed on the commandline). That option doesn't have an _ALIAS, but really shouldn't be used at all, thus we do print a warning. For this particular case we could consider hiding it, but it might be problematic as it doesn't use VSH_OFLAG_REQ_OPT, so is applied without the option name prefix [1] , which could be very misleading to users if it were hidden from the help output. Additionally for that particular case, printing a generic deprecation warning wouln'd IMO be enough as it's really a security issue, not just deprecation. [1]: $ virsh secret-set-value c021dc3e-8227-4bfa-8f1c-ebd0356fb872 YmxlCg== error: Passing secret value as command-line argument is insecure! Secret value set
participants (4)
-
Daniel P. Berrangé -
Michal Privoznik -
Peter Krempa -
Tim Wiederhake