| Message ID | 20180308180907.8592-1-aserdean@ovn.org |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [ovs-dev] ovsdb-client: Set binary on FDs when doing backup/restore | expand |
Please disregard this patch. Copy/paste error 😊 > -----Mesaj original----- > De la: ovs-dev-bounces@openvswitch.org <ovs-dev- > bounces@openvswitch.org> În numele Alin Gabriel Serdean > Trimis: Thursday, March 8, 2018 8:09 PM > Către: dev@openvswitch.org > Cc: Alin Gabriel Serdean <aserdean@ovn.org> > Subiect: [ovs-dev] [PATCH] ovsdb-client: Set binary on FDs when doing > backup/restore > > Add some needed consitentcy on Windows for STD_IN/OUT file descriptors > when doing backup and restore. > > Reported-at:https://mail.openvswitch.org/pipermail/ovs-dev/2018- > January/343518.html > Suggested-by: Ben Pfaff <blp@ovn.org> > Signed-off-by: Alin Gabriel Serdean <aserdean@ovn.org>
On Thu, Mar 08, 2018 at 08:09:07PM +0200, Alin Gabriel Serdean wrote: > Add some needed consitentcy on Windows for STD_IN/OUT file descriptors > when doing backup and restore. > > Reported-at:https://mail.openvswitch.org/pipermail/ovs-dev/2018-January/343518.html > Suggested-by: Ben Pfaff <blp@ovn.org> > Signed-off-by: Alin Gabriel Serdean <aserdean@ovn.org> Thanks! I like this a lot better than the previous approach. I am not sure that I like the idea of combining checking for a tty with setting the mode. I see that we are doing both together, but they seem only loosely related. I guess that I would be happier with a function that does only the mode-set and left the isatty call to the caller. I believe that there is a bug in check_and_set_stdout(): it always works on stdout, ignoring the fd passed in. That matches the "stdout" in its name, but I think it's unintentional. I guess that it should take a FILE *, fflush that stream, and then _setmode() on fileno(stream) (or _fileno(stream))? Something like this: static void set_binary_mode(FILE *stream OVS_UNUSED) { #ifdef _WIN32 fflush(stream); if (_setmode(_fileno(stream), O_BINARY) == -1) { ovs_fatal(errno, "could not set binary mode on fd %d", _fileno(stream)); } #endif } Thanks, Ben.
diff --git a/ovsdb/ovsdb-client.c b/ovsdb/ovsdb-client.c index 222bd6ca8..e1b056df8 100644 --- a/ovsdb/ovsdb-client.c +++ b/ovsdb/ovsdb-client.c @@ -18,6 +18,7 @@ #include <ctype.h> #include <errno.h> +#include <fcntl.h> #include <getopt.h> #include <limits.h> #include <signal.h> @@ -1475,11 +1476,30 @@ print_and_free_log_record(struct json *record) json_destroy(record); } +/* Wrapper to verify if the file descriptor `fd` refers to a terminal. + * On Windows also set binary mode on the file descriptor to avoid + * translation (i.e. CRLF line endings). */ +static bool +check_and_set_stdout(int fd) +{ + if (isatty(fd)) { + return false; + } +#ifdef _WIN32 + fflush(stdout); + int result = _setmode(STDOUT_FILENO, O_BINARY); + if (result == -1) { + ovs_fatal(0, "could not _setmode O_BINARY on STDOUT_FILENO"); + } +#endif + return true; +} + static void do_backup(struct jsonrpc *rpc, const char *database, int argc OVS_UNUSED, char *argv[] OVS_UNUSED) { - if (isatty(STDOUT_FILENO)) { + if (!check_and_set_stdout(STDOUT_FILENO)) { ovs_fatal(0, "not writing backup to a terminal; " "please redirect stdout to a file"); } @@ -1595,7 +1615,7 @@ static void do_restore(struct jsonrpc *rpc, const char *database, int argc OVS_UNUSED, char *argv[] OVS_UNUSED) { - if (isatty(STDIN_FILENO)) { + if (!check_and_set_stdout(STDIN_FILENO)) { ovs_fatal(0, "not reading backup from a terminal; " "please redirect stdin from a file"); }
Add some needed consitentcy on Windows for STD_IN/OUT file descriptors when doing backup and restore. Reported-at:https://mail.openvswitch.org/pipermail/ovs-dev/2018-January/343518.html Suggested-by: Ben Pfaff <blp@ovn.org> Signed-off-by: Alin Gabriel Serdean <aserdean@ovn.org> --- ovsdb/ovsdb-client.c | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-)