[libvirt] [PATCH 2/2] Try stdin for input when no file is specified
Modify virFileReadAll to check for redirected stdin input when no file is specified. This means that not every file argument needs to be required. Signed-off-by: Michael Williams <mspacex@gmail.com> --- src/util/util.c | 10 +++++- tools/virsh.c | 99 +++++++++++++++++++++++++++++++++++-------------------- 2 files changed, 72 insertions(+), 37 deletions(-) diff --git a/src/util/util.c b/src/util/util.c index 554d68e..84b3ae5 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -445,7 +445,15 @@ int virFileReadAll(const char *path, int maxlen, char **buf) { int fd; - if (strcmp(path,"-") == 0) + if (!path) { + if (isatty(fileno(stdin))) { + virReportSystemError(EINVAL, "%s", _("Missing <file> argument")); + return -1; + } else + path = "-"; + } + + if (strcmp(path,"-") == 0) fd = fileno(stdin); else fd = open(path, O_RDONLY); diff --git a/tools/virsh.c b/tools/virsh.c index 76478dc..5b90b72 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -1248,7 +1248,7 @@ static const vshCmdInfo info_create[] = { }; static const vshCmdOptDef opts_create[] = { - {"file", VSH_OT_DATA, VSH_OFLAG_REQ, N_("file containing an XML domain description")}, + {"file", VSH_OT_DATA, 0, N_("file containing an XML domain description")}, #ifndef WIN32 {"console", VSH_OT_BOOL, 0, N_("attach to console after creation")}, #endif @@ -1271,8 +1271,10 @@ cmdCreate(vshControl *ctl, const vshCmd *cmd) if (!vshConnectionUsability(ctl, ctl->conn)) return false; - if (vshCommandOptString(cmd, "file", &from) <= 0) + if (vshCommandOptString(cmd, "file", &from) < 0) { + vshError(ctl, "%s", _("malformed xml argument")); return false; + } if (virFileReadAll(from, VIRSH_MAX_XML_FILE, &buffer) < 0) return false; @@ -1308,7 +1310,7 @@ static const vshCmdInfo info_define[] = { }; static const vshCmdOptDef opts_define[] = { - {"file", VSH_OT_DATA, VSH_OFLAG_REQ, N_("file containing an XML domain description")}, + {"file", VSH_OT_DATA, 0, N_("file containing an XML domain description")}, {NULL, 0, 0, NULL} }; @@ -1323,8 +1325,10 @@ cmdDefine(vshControl *ctl, const vshCmd *cmd) if (!vshConnectionUsability(ctl, ctl->conn)) return false; - if (vshCommandOptString(cmd, "file", &from) <= 0) + if (vshCommandOptString(cmd, "file", &from) < 0) { + vshError(ctl, "%s", _("malformed xml argument")); return false; + } if (virFileReadAll(from, VIRSH_MAX_XML_FILE, &buffer) < 0) return false; @@ -3801,7 +3805,7 @@ static const vshCmdInfo info_domxmlfromnative[] = { static const vshCmdOptDef opts_domxmlfromnative[] = { {"format", VSH_OT_DATA, VSH_OFLAG_REQ, N_("source config data format")}, - {"config", VSH_OT_DATA, VSH_OFLAG_REQ, N_("config data file to import from")}, + {"config", VSH_OT_DATA, 0, N_("config data file to import from")}, {NULL, 0, 0, NULL} }; @@ -3847,7 +3851,7 @@ static const vshCmdInfo info_domxmltonative[] = { static const vshCmdOptDef opts_domxmltonative[] = { {"format", VSH_OT_DATA, VSH_OFLAG_REQ, N_("target config data type format")}, - {"xml", VSH_OT_DATA, VSH_OFLAG_REQ, N_("xml data file to export from")}, + {"xml", VSH_OT_DATA, 0, N_("xml data file to export from")}, {NULL, 0, 0, NULL} }; @@ -4447,8 +4451,10 @@ cmdNetworkCreate(vshControl *ctl, const vshCmd *cmd) if (!vshConnectionUsability(ctl, ctl->conn)) return false; - if (vshCommandOptString(cmd, "file", &from) <= 0) + if (vshCommandOptString(cmd, "file", &from) < 0) { + vshError(ctl, "%s", _("malformed xml argument")); return false; + } if (virFileReadAll(from, VIRSH_MAX_XML_FILE, &buffer) < 0) return false; @@ -4478,7 +4484,7 @@ static const vshCmdInfo info_network_define[] = { }; static const vshCmdOptDef opts_network_define[] = { - {"file", VSH_OT_DATA, VSH_OFLAG_REQ, N_("file containing an XML network description")}, + {"file", VSH_OT_DATA, 0, N_("file containing an XML network description")}, {NULL, 0, 0, NULL} }; @@ -5249,7 +5255,7 @@ static const vshCmdInfo info_interface_define[] = { }; static const vshCmdOptDef opts_interface_define[] = { - {"file", VSH_OT_DATA, VSH_OFLAG_REQ, N_("file containing an XML interface description")}, + {"file", VSH_OT_DATA, 0, N_("file containing an XML interface description")}, {NULL, 0, 0, NULL} }; @@ -5264,8 +5270,10 @@ cmdInterfaceDefine(vshControl *ctl, const vshCmd *cmd) if (!vshConnectionUsability(ctl, ctl->conn)) return false; - if (vshCommandOptString(cmd, "file", &from) <= 0) + if (vshCommandOptString(cmd, "file", &from) < 0) { + vshError(ctl, "%s", _("malformed xml argument")); return false; + } if (virFileReadAll(from, VIRSH_MAX_XML_FILE, &buffer) < 0) return false; @@ -5494,7 +5502,7 @@ static const vshCmdInfo info_nwfilter_define[] = { }; static const vshCmdOptDef opts_nwfilter_define[] = { - {"file", VSH_OT_DATA, VSH_OFLAG_REQ, N_("file containing an XML network filter description")}, + {"file", VSH_OT_DATA, 0, N_("file containing an XML network filter description")}, {NULL, 0, 0, NULL} }; @@ -5509,8 +5517,10 @@ cmdNWFilterDefine(vshControl *ctl, const vshCmd *cmd) if (!vshConnectionUsability(ctl, ctl->conn)) return false; - if (vshCommandOptString(cmd, "file", &from) <= 0) + if (vshCommandOptString(cmd, "file", &from) < 0) { + vshError(ctl, "%s", _("malformed xml argument")); return false; + } if (virFileReadAll(from, VIRSH_MAX_XML_FILE, &buffer) < 0) return false; @@ -5832,7 +5842,7 @@ static const vshCmdInfo info_pool_create[] = { }; static const vshCmdOptDef opts_pool_create[] = { - {"file", VSH_OT_DATA, VSH_OFLAG_REQ, + {"file", VSH_OT_DATA, 0, N_("file containing an XML pool description")}, {NULL, 0, 0, NULL} }; @@ -5848,8 +5858,10 @@ cmdPoolCreate(vshControl *ctl, const vshCmd *cmd) if (!vshConnectionUsability(ctl, ctl->conn)) return false; - if (vshCommandOptString(cmd, "file", &from) <= 0) + if (vshCommandOptString(cmd, "file", &from) < 0) { + vshError(ctl, "%s", _("malformed xml argument")); return false; + } if (virFileReadAll(from, VIRSH_MAX_XML_FILE, &buffer) < 0) return false; @@ -5882,7 +5894,7 @@ static const vshCmdInfo info_node_device_create[] = { }; static const vshCmdOptDef opts_node_device_create[] = { - {"file", VSH_OT_DATA, VSH_OFLAG_REQ, + {"file", VSH_OT_DATA, 0, N_("file containing an XML description of the device")}, {NULL, 0, 0, NULL} }; @@ -5898,8 +5910,10 @@ cmdNodeDeviceCreate(vshControl *ctl, const vshCmd *cmd) if (!vshConnectionUsability(ctl, ctl->conn)) return false; - if (vshCommandOptString(cmd, "file", &from) <= 0) + if (vshCommandOptString(cmd, "file", &from) < 0) { + vshError(ctl, "%s", _("malformed xml argument")); return false; + } if (virFileReadAll(from, VIRSH_MAX_XML_FILE, &buffer) < 0) return false; @@ -6092,7 +6106,7 @@ static const vshCmdInfo info_pool_define[] = { }; static const vshCmdOptDef opts_pool_define[] = { - {"file", VSH_OT_DATA, VSH_OFLAG_REQ, N_("file containing an XML pool description")}, + {"file", VSH_OT_DATA, 0, N_("file containing an XML pool description")}, {NULL, 0, 0, NULL} }; @@ -6107,8 +6121,10 @@ cmdPoolDefine(vshControl *ctl, const vshCmd *cmd) if (!vshConnectionUsability(ctl, ctl->conn)) return false; - if (vshCommandOptString(cmd, "file", &from) <= 0) + if (vshCommandOptString(cmd, "file", &from) < 0) { + vshError(ctl, "%s", _("malformed xml argument")); return false; + } if (virFileReadAll(from, VIRSH_MAX_XML_FILE, &buffer) < 0) return false; @@ -7338,7 +7354,7 @@ static const vshCmdInfo info_vol_create[] = { static const vshCmdOptDef opts_vol_create[] = { {"pool", VSH_OT_DATA, VSH_OFLAG_REQ, N_("pool name")}, - {"file", VSH_OT_DATA, VSH_OFLAG_REQ, N_("file containing an XML vol description")}, + {"file", VSH_OT_DATA, 0, N_("file containing an XML vol description")}, {NULL, 0, 0, NULL} }; @@ -7358,7 +7374,8 @@ cmdVolCreate(vshControl *ctl, const vshCmd *cmd) VSH_BYNAME))) return false; - if (vshCommandOptString(cmd, "file", &from) <= 0) { + if (vshCommandOptString(cmd, "file", &from) < 0) { + vshError(ctl, "%s", _("malformed xml argument")); virStoragePoolFree(pool); return false; } @@ -7395,7 +7412,7 @@ static const vshCmdInfo info_vol_create_from[] = { static const vshCmdOptDef opts_vol_create_from[] = { {"pool", VSH_OT_DATA, VSH_OFLAG_REQ, N_("pool name")}, - {"file", VSH_OT_DATA, VSH_OFLAG_REQ, N_("file containing an XML vol description")}, + {"file", VSH_OT_DATA, 0, N_("file containing an XML vol description")}, {"vol", VSH_OT_DATA, VSH_OFLAG_REQ, N_("input vol name or key")}, {"inputpool", VSH_OT_STRING, 0, N_("pool name or uuid of the input volume's pool")}, {NULL, 0, 0, NULL} @@ -7416,7 +7433,8 @@ cmdVolCreateFrom(vshControl *ctl, const vshCmd *cmd) if (!(pool = vshCommandOptPoolBy(ctl, cmd, "pool", NULL, VSH_BYNAME))) goto cleanup; - if (vshCommandOptString(cmd, "file", &from) <= 0) { + if (vshCommandOptString(cmd, "file", &from) < 0) { + vshError(ctl, "%s", _("malformed xml argument")); goto cleanup; } @@ -7570,7 +7588,7 @@ static const vshCmdInfo info_vol_upload[] = { static const vshCmdOptDef opts_vol_upload[] = { {"vol", VSH_OT_DATA, VSH_OFLAG_REQ, N_("vol name, key or path")}, - {"file", VSH_OT_DATA, VSH_OFLAG_REQ, N_("file")}, + {"file", VSH_OT_DATA, 0, N_("file")}, {"pool", VSH_OT_STRING, 0, N_("pool name or uuid")}, {"offset", VSH_OT_INT, 0, N_("volume offset to upload to") }, {"length", VSH_OT_INT, 0, N_("amount of data to upload") }, @@ -7670,7 +7688,7 @@ static const vshCmdInfo info_vol_download[] = { static const vshCmdOptDef opts_vol_download[] = { {"vol", VSH_OT_DATA, VSH_OFLAG_REQ, N_("vol name, key or path")}, - {"file", VSH_OT_DATA, VSH_OFLAG_REQ, N_("file")}, + {"file", VSH_OT_DATA, 0, N_("file")}, {"pool", VSH_OT_STRING, 0, N_("pool name or uuid")}, {"offset", VSH_OT_INT, 0, N_("volume offset to download from") }, {"length", VSH_OT_INT, 0, N_("amount of data to download") }, @@ -8380,7 +8398,7 @@ static const vshCmdInfo info_secret_define[] = { }; static const vshCmdOptDef opts_secret_define[] = { - {"file", VSH_OT_DATA, VSH_OFLAG_REQ, N_("file containing secret attributes in XML")}, + {"file", VSH_OT_DATA, 0, N_("file containing secret attributes in XML")}, {NULL, 0, 0, NULL} }; @@ -8395,8 +8413,10 @@ cmdSecretDefine(vshControl *ctl, const vshCmd *cmd) if (!vshConnectionUsability(ctl, ctl->conn)) return false; - if (vshCommandOptString(cmd, "file", &from) <= 0) + if (vshCommandOptString(cmd, "file", &from) < 0) { + vshError(ctl, "%s", _("malformed xml argument")); return false; + } if (virFileReadAll(from, VIRSH_MAX_XML_FILE, &buffer) < 0) return false; @@ -9325,7 +9345,7 @@ static const vshCmdInfo info_attach_device[] = { static const vshCmdOptDef opts_attach_device[] = { {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, - {"file", VSH_OT_DATA, VSH_OFLAG_REQ, N_("XML file")}, + {"file", VSH_OT_DATA, 0, N_("XML file")}, {"persistent", VSH_OT_BOOL, 0, N_("persist device attachment")}, {NULL, 0, 0, NULL} }; @@ -9345,7 +9365,8 @@ cmdAttachDevice(vshControl *ctl, const vshCmd *cmd) if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) return false; - if (vshCommandOptString(cmd, "file", &from) <= 0) { + if (vshCommandOptString(cmd, "file", &from) < 0) { + vshError(ctl, "%s", _("malformed xml argument")); virDomainFree(dom); return false; } @@ -9390,7 +9411,7 @@ static const vshCmdInfo info_detach_device[] = { static const vshCmdOptDef opts_detach_device[] = { {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, - {"file", VSH_OT_DATA, VSH_OFLAG_REQ, N_("XML file")}, + {"file", VSH_OT_DATA, 0, N_("XML file")}, {"persistent", VSH_OT_BOOL, 0, N_("persist device detachment")}, {NULL, 0, 0, NULL} }; @@ -9410,7 +9431,8 @@ cmdDetachDevice(vshControl *ctl, const vshCmd *cmd) if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) return false; - if (vshCommandOptString(cmd, "file", &from) <= 0) { + if (vshCommandOptString(cmd, "file", &from) < 0) { + vshError(ctl, "%s", _("malformed xml argument")); virDomainFree(dom); return false; } @@ -9455,7 +9477,7 @@ static const vshCmdInfo info_update_device[] = { static const vshCmdOptDef opts_update_device[] = { {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, - {"file", VSH_OT_DATA, VSH_OFLAG_REQ, N_("XML file")}, + {"file", VSH_OT_DATA, 0, N_("XML file")}, {"persistent", VSH_OT_BOOL, 0, N_("persist device update")}, {"force", VSH_OT_BOOL, 0, N_("force device update")}, {NULL, 0, 0, NULL} @@ -9476,7 +9498,8 @@ cmdUpdateDevice(vshControl *ctl, const vshCmd *cmd) if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) return false; - if (vshCommandOptString(cmd, "file", &from) <= 0) { + if (vshCommandOptString(cmd, "file", &from) < 0) { + vshError(ctl, "%s", _("malformed xml argument")); virDomainFree(dom); return false; } @@ -10042,7 +10065,7 @@ static const vshCmdInfo info_cpu_compare[] = { }; static const vshCmdOptDef opts_cpu_compare[] = { - {"file", VSH_OT_DATA, VSH_OFLAG_REQ, N_("file containing an XML CPU description")}, + {"file", VSH_OT_DATA, 0, N_("file containing an XML CPU description")}, {NULL, 0, 0, NULL} }; @@ -10057,8 +10080,10 @@ cmdCPUCompare(vshControl *ctl, const vshCmd *cmd) if (!vshConnectionUsability(ctl, ctl->conn)) return false; - if (vshCommandOptString(cmd, "file", &from) <= 0) + if (vshCommandOptString(cmd, "file", &from) < 0) { + vshError(ctl, "%s", _("malformed xml argument")); return false; + } if (virFileReadAll(from, VIRSH_MAX_XML_FILE, &buffer) < 0) return false; @@ -10104,7 +10129,7 @@ static const vshCmdInfo info_cpu_baseline[] = { }; static const vshCmdOptDef opts_cpu_baseline[] = { - {"file", VSH_OT_DATA, VSH_OFLAG_REQ, N_("file containing XML CPU descriptions")}, + {"file", VSH_OT_DATA, 0, N_("file containing XML CPU descriptions")}, {NULL, 0, 0, NULL} }; @@ -10128,8 +10153,10 @@ cmdCPUBaseline(vshControl *ctl, const vshCmd *cmd) if (!vshConnectionUsability(ctl, ctl->conn)) return false; - if (vshCommandOptString(cmd, "file", &from) <= 0) + if (vshCommandOptString(cmd, "file", &from) < 0) { + vshError(ctl, "%s", _("malformed xml argument")); return false; + } if (virFileReadAll(from, VIRSH_MAX_XML_FILE, &buffer) < 0) return false; -- 1.7.3.4
On 06/13/2011 09:39 PM, Michael Williams wrote:
Modify virFileReadAll to check for redirected stdin input when no file is specified. This means that not every file argument needs to be required.
Signed-off-by: Michael Williams <mspacex@gmail.com> --- src/util/util.c | 10 +++++- tools/virsh.c | 99 +++++++++++++++++++++++++++++++++++-------------------- 2 files changed, 72 insertions(+), 37 deletions(-)
diff --git a/src/util/util.c b/src/util/util.c index 554d68e..84b3ae5 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -445,7 +445,15 @@ int virFileReadAll(const char *path, int maxlen, char **buf) { int fd; - if (strcmp(path,"-") == 0) + if (!path) {
Your mailer uses odd spacing.
+ if (isatty(fileno(stdin))) {
Same comments about using STDIN_FILENO. I'm debating whether this line belongs here, or whether it belongs better in virsh.c. That is, I don't know whether this is a virsh-specific feature, or whether all of the libvirt library should behave like this. I'm also wondering if the tty check is correct; it seems like: 'virsh define' should be able to read from stdin (it is a batch run rather than interactive, but has just one command, so it is okay if that command consumes stdin); whereas: 'virsh' followed by 'define' must not consume stdin, whether or not stdin is a tty (that is, interactive mode must explicitly request "-" as the filename) and: 'virsh "define; list"' should not consume stdin (batch mode with more than one command, and the first command should not need to look ahead to see whether subsequent commands might want to use stdin). I'm not even sure if stdin being a tty is the right check.
+ virReportSystemError(EINVAL, "%s", _("Missing <file> argument")); + return -1; + } else + path = "-"; + } + + if (strcmp(path,"-") == 0) fd = fileno(stdin);
Formatting - this should be two lines.
@@ -1323,8 +1325,10 @@ cmdDefine(vshControl *ctl, const vshCmd *cmd) if (!vshConnectionUsability(ctl, ctl->conn)) return false; - if (vshCommandOptString(cmd, "file", &from) <= 0) + if (vshCommandOptString(cmd, "file", &from) < 0) { + vshError(ctl, "%s", _("malformed xml argument"));
Not quite the right message; "malformed xml" implies that the file existed and was read but could not be parsed. Really, the error here is that someone called --file '' (that is, the empty string), so a better error is "empty xml argument". But overall, the idea is quite nice. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Changes made, will submit an updated patchset after finalizing the rest of this patch. On 6/13/11 11:06 PM, Eric Blake wrote:
I'm debating whether this line belongs here, or whether it belongs better in virsh.c. That is, I don't know whether this is a virsh-specific feature, or whether all of the libvirt library should behave like this.
This very well could be only be needed in virsh, it just seemed like util/util.c was where it could have the least duplication of code.
I'm not even sure if stdin being a tty is the right check.
isatty() produces the correct behavior in all cases but the first one you mentioned, ie running "virsh define", which will not wait for input with the patch as it is. I see the ctl->imode parameter used in main() to determine the interactivity of the session, though that is only accessible at the cmdOPERATION level, not in virFileReadAll(). Maybe (yet) another read wrapper local to virsh.c that would check the ctl->imode if no path is offered, and make a decision from there. Thoughts? -- ~Michael Williams
On 06/14/2011 12:12 AM, Michael Williams wrote:
isatty() produces the correct behavior in all cases but the first one you mentioned, ie running "virsh define", which will not wait for input with the patch as it is.
I see the ctl->imode parameter used in main() to determine the interactivity of the session, though that is only accessible at the cmdOPERATION level, not in virFileReadAll(). Maybe (yet) another read wrapper local to virsh.c that would check the ctl->imode if no path is offered, and make a decision from there.
Yeah, that seems best. Basically, a function in virsh that does something like (untested): /* If the named option is present, read that file into * buffer; if the option is not present, then decide whether * to read stdin or give an error. */ int vshFileRead(vshControl *ctl, const vshCmd *cmd, const char *option, char **buffer) { char *file = NULL; int ret = vshCommandOptString(cmd, option, &file); if (ret < 0) { vshError(ctl, _("option %s must not be empty"), option); return ret; } if (ret == 0) { if (ctl->lastCommand || (ctl->imode && isatty(STDIN_FILENO))) { file = "-"; } else { vshError(ctl, _("refusing to use implicit stdin")); return -1; } } return virFileReadAll(file, limit, buffer); } and which would replace this code: - if (vshCommandOptString(cmd, "file", &from) <= 0) + if (vshCommandOptString(cmd, "file", &from) < 0) { + vshError(ctl, "%s", _("malformed xml argument")); return false; + } if (virFileReadAll(from, VIRSH_MAX_XML_FILE, &buffer) < 0) return false; with the simpler: if (vshFileRead(ctl, cmd, "file", &buffer) < 0) return false; That proposal also implies adding a new ctl->lastCommand flag, which tracks whether a given command is the last command given to virsh. That flag would default to false, and be set to true when you use the one-command 'virsh command arg' form, or when you use the multi-command 'virsh "command; command"' form and the parser detects the end of the input string before issuing the last command. Hmm, looking at my above example, it looks like it is indeed easier to put the "-" logic into virFileReadAll, after all, but that virFileReadAll must always be given a non-NULL filename (that is, the conversion from NULL to "-" is done in virsh, but the conversion from "-" to stdin is done in util). -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Michael Williams