Re: [libvirt] [PATCH 3/3] Use virBufferEscapeShell in virNetSocketNewConnectSSH

2011-10-13 Thread Guido Günther 
On Wed, Oct 12, 2011 at 05:22:02PM -0600, Eric Blake wrote:
 On 10/12/2011 04:40 PM, Guido Günther wrote:
 to escape the netcat command since it's passed to the shell. Adjust
 expected test case output accordingly.
 ---
   src/rpc/virnetsocket.c   |   25 -
   tests/virnetsockettest.c |   10 +-
   2 files changed, 25 insertions(+), 10 deletions(-)
 
 ACK with nits fixed.  Still to go - it would be nice to follow up
 with a 4/3 that converts virsh.c cmdEcho to use
 virBufferEscapeShell.
 
 @@ -636,6 +639,19 @@ int virNetSocketNewConnectSSH(const char *nodename,
   virCommandAddArgList(cmd, -o, StrictHostKeyChecking=no, NULL);
 
   virCommandAddArgList(cmd, nodename, sh, -c, NULL);
 +
 +virBufferEscapeShell(buf, netcat ? netcat : nc);
 +if (virBufferError(buf)) {
 +virBufferFreeAndReset(buf);
 +virReportOOMError();
 +return -1;
 +}
 +quoted = virBufferContentAndReset(buf);
 +if (quoted == NULL) {
 
 You are guaranteed that quoted is not NULL, by virtue of the fact
 that you already filtered for virBufferError just beforehand.  Drop
 this if statement.

Done.

 
 +++ b/tests/virnetsockettest.c
 @@ -496,7 +496,7 @@ mymain(void)
   struct testSSHData sshData1 = {
   .nodename = somehost,
   .path = /tmp/socket,
 -.expectOut = somehost sh -c 'if nc -q 21 | grep \requires an 
 argument\/dev/null 21; then ARG=-q0;fi;nc $ARG -U /tmp/socket'\n,
 +.expectOut = somehost sh -c 'if 'nc' -q 21 | grep \requires an 
 argument\/dev/null 21; then ARG=-q0;fi;'nc' $ARG -U /tmp/socket'\n,
   };
 
 Rebase this on top of my comments for 1/3 (no long lines).  Also,
 add one more test where $NC is something that actually proves things
 work with shell meta-characters, perhaps by passing 'nc -4' as the
 value that eventually supplies the %s for $NC, so that .expectOut
 will include: sh -c 'if ''nc -4'' -q 21 

Done too.
Cheers,
 -- Guido

 
 -- 
 Eric Blake   ebl...@redhat.com+1-801-349-2682
 Libvirt virtualization library http://libvirt.org
 
From 48650440f98ea4f975d71f97c5c050a1a4653690 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Guido=20G=C3=BCnther?= a...@sigxcpu.org
Date: Thu, 13 Oct 2011 21:49:01 +0200
Subject: [PATCH 3/4] Use virBufferEscapeShell in virNetSocketNewConnectSSH

to escape the netcat command since it's passed to the shell. Adjust
expected test case output accordingly.
---
 src/rpc/virnetsocket.c   |   18 +++---
 tests/virnetsockettest.c |   34 --
 2 files changed, 39 insertions(+), 13 deletions(-)

diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c
index 2a9bca0..e4eff49 100644
--- a/src/rpc/virnetsocket.c
+++ b/src/rpc/virnetsocket.c
@@ -612,7 +612,10 @@ int virNetSocketNewConnectSSH(const char *nodename,
   const char *path,
   virNetSocketPtr *retsock)
 {
+char *quoted;
 virCommandPtr cmd;
+virBuffer buf = VIR_BUFFER_INITIALIZER;
+
 *retsock = NULL;
 
 cmd = virCommandNew(binary ? binary : ssh);
@@ -639,6 +642,14 @@ int virNetSocketNewConnectSSH(const char *nodename,
 netcat = nc;
 
 virCommandAddArgList(cmd, nodename, sh, -c, NULL);
+
+virBufferEscapeShell(buf, netcat);
+if (virBufferError(buf)) {
+virBufferFreeAndReset(buf);
+virReportOOMError();
+return -1;
+}
+quoted = virBufferContentAndReset(buf);
 /*
  * This ugly thing is a shell script to detect availability of
  * the -q option for 'nc': debian and suse based distros need this
@@ -650,14 +661,15 @@ int virNetSocketNewConnectSSH(const char *nodename,
  * behavior.
  */
 virCommandAddArgFormat(cmd,
- 'if %s -q 21 | grep \requires an argument\ /dev/null 21; then 
+ 'if '%s' -q 21 | grep \requires an argument\ /dev/null 21; then 
  ARG=-q0;
  else 
  ARG=;
  fi;
- %s $ARG -U %s',
- netcat, netcat, path);
+ '%s' $ARG -U %s',
+ quoted, quoted, path);
 
+VIR_FREE(quoted);
 return virNetSocketNewConnectCommand(cmd, retsock);
 }
 
diff --git a/tests/virnetsockettest.c b/tests/virnetsockettest.c
index 75cc9c0..6320ce0 100644
--- a/tests/virnetsockettest.c
+++ b/tests/virnetsockettest.c
@@ -496,12 +496,12 @@ mymain(void)
 struct testSSHData sshData1 = {
 .nodename = somehost,
 .path = /tmp/socket,
-.expectOut = somehost sh -c 'if nc -q 21 | grep \requires an argument\ /dev/null 21; then 
+.expectOut = somehost sh -c 'if 'nc' -q 21 | grep \requires an argument\ /dev/null 21; then 
  ARG=-q0;
  else 
  ARG=;
  fi;
- nc $ARG -U /tmp/socket'\n,
+ 'nc' $ARG -U /tmp/socket'\n,

Re: [libvirt] [PATCH 3/3] Use virBufferEscapeShell in virNetSocketNewConnectSSH

2011-10-13 Thread Eric Blake 

On 10/13/2011 03:02 PM, Guido Günther wrote:

On Wed, Oct 12, 2011 at 05:22:02PM -0600, Eric Blake wrote:

On 10/12/2011 04:40 PM, Guido Günther wrote:

to escape the netcat command since it's passed to the shell. Adjust
expected test case output accordingly.
---


Rebase this on top of my comments for 1/3 (no long lines).  Also,
add one more test where $NC is something that actually proves things
work with shell meta-characters, perhaps by passing 'nc -4' as the
value that eventually supplies the %s for $NC, so that .expectOut
will include: sh -c 'if ''nc -4'' -q 21 


Done too.


ACK.

--
Eric Blake   ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 3/3] Use virBufferEscapeShell in virNetSocketNewConnectSSH

2011-10-12 Thread Guido Günther 
to escape the netcat command since it's passed to the shell. Adjust
expected test case output accordingly.
---
 src/rpc/virnetsocket.c   |   25 -
 tests/virnetsockettest.c |   10 +-
 2 files changed, 25 insertions(+), 10 deletions(-)

diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c
index ea653da..0105e45 100644
--- a/src/rpc/virnetsocket.c
+++ b/src/rpc/virnetsocket.c
@@ -612,7 +612,10 @@ int virNetSocketNewConnectSSH(const char *nodename,
   const char *path,
   virNetSocketPtr *retsock)
 {
+char *quoted;
 virCommandPtr cmd;
+virBuffer buf = VIR_BUFFER_INITIALIZER;
+
 *retsock = NULL;
 
 cmd = virCommandNew(binary ? binary : ssh);
@@ -636,6 +639,19 @@ int virNetSocketNewConnectSSH(const char *nodename,
 virCommandAddArgList(cmd, -o, StrictHostKeyChecking=no, NULL);
 
 virCommandAddArgList(cmd, nodename, sh, -c, NULL);
+
+virBufferEscapeShell(buf, netcat ? netcat : nc);
+if (virBufferError(buf)) {
+virBufferFreeAndReset(buf);
+virReportOOMError();
+return -1;
+}
+quoted = virBufferContentAndReset(buf);
+if (quoted == NULL) {
+virReportOOMError();
+return -1;
+}
+
 /*
  * This ugly thing is a shell script to detect availability of
  * the -q option for 'nc': debian and suse based distros need this
@@ -647,14 +663,13 @@ int virNetSocketNewConnectSSH(const char *nodename,
  * behavior.
  */
 virCommandAddArgFormat(cmd,
- 'if %s -q 21 | grep \requires an argument\ /dev/null 21; then
+ 'if '%s' -q 21 | grep \requires an argument\ /dev/null 21; 
then
   ARG=-q0;
  fi;
- %s $ARG -U %s',
- netcat ? netcat : nc,
- netcat ? netcat : nc,
- path);
+ '%s' $ARG -U %s',
+ quoted, quoted, path);
 
+VIR_FREE(quoted);
 return virNetSocketNewConnectCommand(cmd, retsock);
 }
 
diff --git a/tests/virnetsockettest.c b/tests/virnetsockettest.c
index b3a2705..c063e74 100644
--- a/tests/virnetsockettest.c
+++ b/tests/virnetsockettest.c
@@ -496,7 +496,7 @@ mymain(void)
 struct testSSHData sshData1 = {
 .nodename = somehost,
 .path = /tmp/socket,
-.expectOut = somehost sh -c 'if nc -q 21 | grep \requires an 
argument\ /dev/null 21; then ARG=-q0;fi;nc $ARG -U /tmp/socket'\n,
+.expectOut = somehost sh -c 'if 'nc' -q 21 | grep \requires an 
argument\ /dev/null 21; then ARG=-q0;fi;'nc' $ARG -U /tmp/socket'\n,
 };
 if (virtTestRun(SSH test 1, 1, testSocketSSH, sshData1)  0)
 ret = -1;
@@ -509,7 +509,7 @@ mymain(void)
 .noTTY = true,
 .noVerify = false,
 .path = /tmp/socket,
-.expectOut = -p 9000 -l fred -T -o BatchMode=yes -e none somehost sh 
-c 'if netcat -q 21 | grep \requires an argument\ /dev/null 21; then 
ARG=-q0;fi;netcat $ARG -U /tmp/socket'\n,
+.expectOut = -p 9000 -l fred -T -o BatchMode=yes -e none somehost sh 
-c 'if 'netcat' -q 21 | grep \requires an argument\ /dev/null 21; then   
  ARG=-q0;fi;'netcat' $ARG -U /tmp/socket'\n,
 };
 if (virtTestRun(SSH test 2, 1, testSocketSSH, sshData2)  0)
 ret = -1;
@@ -522,7 +522,7 @@ mymain(void)
 .noTTY = false,
 .noVerify = true,
 .path = /tmp/socket,
-.expectOut = -p 9000 -l fred -o StrictHostKeyChecking=no somehost sh 
-c 'if netcat -q 21 | grep \requires an argument\ /dev/null 21; then 
ARG=-q0;fi;netcat $ARG -U /tmp/socket'\n,
+.expectOut = -p 9000 -l fred -o StrictHostKeyChecking=no somehost sh 
-c 'if 'netcat' -q 21 | grep \requires an argument\ /dev/null 21; then   
  ARG=-q0;fi;'netcat' $ARG -U /tmp/socket'\n,
 };
 if (virtTestRun(SSH test 3, 1, testSocketSSH, sshData3)  0)
 ret = -1;
@@ -538,7 +538,7 @@ mymain(void)
 struct testSSHData sshData5 = {
 .nodename = crashyhost,
 .path = /tmp/socket,
-.expectOut = crashyhost sh -c 'if nc -q 21 | grep \requires an 
argument\ /dev/null 21; then ARG=-q0;fi;nc $ARG -U /tmp/socket'\n,
+.expectOut = crashyhost sh -c 'if 'nc' -q 21 | grep \requires an 
argument\ /dev/null 21; then ARG=-q0;fi;'nc' $ARG -U /tmp/socket'\n,
 
 .dieEarly = true,
 };
@@ -550,7 +550,7 @@ mymain(void)
 .path = /tmp/socket,
 .keyfile = /root/.ssh/example_key,
 .noVerify = true,
-.expectOut = -i /root/.ssh/example_key -o StrictHostKeyChecking=no 
example.com sh -c 'if nc -q 21 | grep \requires an argument\ /dev/null 
21; then ARG=-q0;fi;nc $ARG -U /tmp/socket'\n,
+.expectOut = -i /root/.ssh/example_key -o StrictHostKeyChecking=no 
example.com sh -c 'if 'nc' -q 21 | grep \requires an argument\ /dev/null 
21; then ARG=-q0;fi;'nc' $ARG -U /tmp/socket'\n,
 };
 if (virtTestRun(SSH test 6, 1, testSocketSSH, sshData6)  0)
 ret = -1;
-- 
1.7.6.3

--
libvir-list mailing

Re: [libvirt] [PATCH 3/3] Use virBufferEscapeShell in virNetSocketNewConnectSSH

2011-10-12 Thread Eric Blake 

On 10/12/2011 04:40 PM, Guido Günther wrote:

to escape the netcat command since it's passed to the shell. Adjust
expected test case output accordingly.
---
  src/rpc/virnetsocket.c   |   25 -
  tests/virnetsockettest.c |   10 +-
  2 files changed, 25 insertions(+), 10 deletions(-)


ACK with nits fixed.  Still to go - it would be nice to follow up with a 
4/3 that converts virsh.c cmdEcho to use virBufferEscapeShell.



@@ -636,6 +639,19 @@ int virNetSocketNewConnectSSH(const char *nodename,
  virCommandAddArgList(cmd, -o, StrictHostKeyChecking=no, NULL);

  virCommandAddArgList(cmd, nodename, sh, -c, NULL);
+
+virBufferEscapeShell(buf, netcat ? netcat : nc);
+if (virBufferError(buf)) {
+virBufferFreeAndReset(buf);
+virReportOOMError();
+return -1;
+}
+quoted = virBufferContentAndReset(buf);
+if (quoted == NULL) {


You are guaranteed that quoted is not NULL, by virtue of the fact that 
you already filtered for virBufferError just beforehand.  Drop this if 
statement.



+++ b/tests/virnetsockettest.c
@@ -496,7 +496,7 @@ mymain(void)
  struct testSSHData sshData1 = {
  .nodename = somehost,
  .path = /tmp/socket,
-.expectOut = somehost sh -c 'if nc -q 21 | grep \requires an 
argument\/dev/null 21; then ARG=-q0;fi;nc $ARG -U /tmp/socket'\n,
+.expectOut = somehost sh -c 'if 'nc' -q 21 | grep \requires an 
argument\/dev/null 21; then ARG=-q0;fi;'nc' $ARG -U /tmp/socket'\n,
  };


Rebase this on top of my comments for 1/3 (no long lines).  Also, add 
one more test where $NC is something that actually proves things work 
with shell meta-characters, perhaps by passing 'nc -4' as the value that 
eventually supplies the %s for $NC, so that .expectOut will include: sh 
-c 'if ''nc -4'' -q 21 


--
Eric Blake   ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list