Re: [libvirt] [PATCH 3/3] Get QEMU pty paths from the monitor

2009-12-14 Thread Daniel Veillard 
On Thu, Dec 10, 2009 at 01:41:39PM +, Matthew Booth wrote:
 This change makes the QEMU driver get pty paths from the output of the monitor
 'info chardev' command. This output is structured, and contains both the name 
 of
 the device and the path on the same line. This is considerably more reliable
 than parsing the startup log output, which requires the parsing code to know
 which order QEMU will print pty information in.
 
 Note that we still need to parse the log output as the monitor itself may be 
 on
 a pty. This should be rare, however, and the new code will replace all pty 
 paths
 parsed by the log output method once the monitor is available.
 
 * src/qemu/qemu_monitor.(c|h) src/qemu_monitor_text.(c|h): Implement
   qemuMonitorGetPtyPaths().
 * src/qemu/qemu_driver.c: Get pty path information using
   qemuMonitorGetPtyPaths().

  Okay, I have pushed the 3 patches, code looks fine, and lot of
regression tests ! I just changed that last patch to reformat the
macro and replaced a free() into VIR_FREE() ... IIRC those were pointed
by danpb in his last review.

  thanks !

Daniel

-- 
Daniel Veillard  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
dan...@veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/

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

Re: [libvirt] [PATCH 3/3] Get QEMU pty paths from the monitor

2009-12-14 Thread Daniel Veillard 
On Wed, Nov 25, 2009 at 11:31:37AM +, Daniel P. Berrange wrote:
 On Mon, Nov 23, 2009 at 12:30:29PM +, Matthew Booth wrote:
  @@ -1291,7 +1330,7 @@ qemudWaitForMonitor(virConnectPtr conn,
   {
   char buf[4096]; /* Plenty of space to get startup greeting */
   int logfd;
  -int ret;
  +int ret = -1;
   
   if ((logfd = qemudLogReadFD(conn, driver-logDir, vm-def-name, pos))
0)
  @@ -1317,7 +1356,32 @@ qemudWaitForMonitor(virConnectPtr conn,
   if (qemuConnectMonitor(vm)  0)
   return -1;
   
  -return 0;
  +/* Try to get the pty path mappings again via the monitor. This is 
  much more
  + * reliable if it's available.
  + * Note that the monitor itself can be on a pty, so we still need to 
  try the
  + * log output method. */
  +virHashTablePtr paths = virHashCreate(0);
  +if (paths == NULL) {
  +virReportOOMError(NULL);
  +goto cleanup;
  +}
  +
  +qemuDomainObjEnterMonitor(vm);
 
 This needs to be EnterMonitorWithDriver(driver, vm), since the 'driver'
 is locked in this context
 
  +qemuDomainObjPrivatePtr priv = vm-privateData;
  +ret = qemuMonitorGetPtyPaths(priv-mon, paths);
  +qemuDomainObjExitMonitor(vm);
 
 And ExitMonitorWithDriver


  Loooks serious and was missing fro last patch so I made the change


  +/* Path is everything after needle to the end of the line */
  +*eol = '\0';
  +char *path = needle + strlen(NEEDLE);
  +
  +virHashAddEntry(paths, id, strdup(path));
 
 Not checking OOM on strdup() here, or for failure of virHashAddEntry()

  I fixed that too, I pushed the attached patch to clean those 2 issues,
please check :-)

Daniel

-- 
Daniel Veillard  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
dan...@veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/
commit a7d1eb3c446fdbbd4183ba398c7f21a9fc0c1696
Author: Daniel Veillard veill...@redhat.com
Date:   Mon Dec 14 11:05:55 2009 +0100

Fix a couple of problems in last patch

Those were pointed by DanB in his review but not yet fixed

* src/qemu/qemu_driver.c: qemudWaitForMonitor() use EnterMonitorWithDriver()
  and ExitMonitorWithDriver() there
* src/qemu/qemu_monitor_text.c: checking fro strdu failure and hash
  table add error in qemuMonitorTextGetPtyPaths()

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 7b8c447..5a61b8c 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1534,10 +1534,10 @@ qemudWaitForMonitor(virConnectPtr conn,
 goto cleanup;
 }
 
-qemuDomainObjEnterMonitor(vm);
+qemuDomainObjEnterMonitorWithDriver(driver, vm);
 qemuDomainObjPrivatePtr priv = vm-privateData;
 ret = qemuMonitorGetPtyPaths(priv-mon, paths);
-qemuDomainObjExitMonitor(vm);
+qemuDomainObjExitMonitorWithDriver(driver, vm);
 
 VIR_DEBUG(qemuMonitorGetPtyPaths returned %i, ret);
 if (ret == 0) {
diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c
index 3ed45ba..0cb9ea6 100644
--- a/src/qemu/qemu_monitor_text.c
+++ b/src/qemu/qemu_monitor_text.c
@@ -1650,9 +1650,19 @@ int qemuMonitorTextGetPtyPaths(qemuMonitorPtr mon,
 
 /* Path is everything after needle to the end of the line */
 *eol = '\0';
-char *path = needle + strlen(NEEDLE);
+char *path = strdup(needle + strlen(NEEDLE));
+if (path == NULL) {
+virReportOOMError(NULL);
+goto cleanup;
+}
 
-virHashAddEntry(paths, id, strdup(path));
+if (virHashAddEntry(paths, id, path)  0) {
+qemudReportError(NULL, NULL, NULL, VIR_ERR_OPERATION_FAILED,
+ _(failed to save chardev path '%s'),
+ path);
+VIR_FREE(path);
+goto cleanup;
+}
 #undef NEEDLE
 
 next:
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 3/3] Get QEMU pty paths from the monitor

2009-12-10 Thread Matthew Booth 
This change makes the QEMU driver get pty paths from the output of the monitor
'info chardev' command. This output is structured, and contains both the name of
the device and the path on the same line. This is considerably more reliable
than parsing the startup log output, which requires the parsing code to know
which order QEMU will print pty information in.

Note that we still need to parse the log output as the monitor itself may be on
a pty. This should be rare, however, and the new code will replace all pty paths
parsed by the log output method once the monitor is available.

* src/qemu/qemu_monitor.(c|h) src/qemu_monitor_text.(c|h): Implement
  qemuMonitorGetPtyPaths().
* src/qemu/qemu_driver.c: Get pty path information using
  qemuMonitorGetPtyPaths().
---
 src/qemu/qemu_driver.c   |   68 +++-
 src/qemu/qemu_monitor.c  |9 +
 src/qemu/qemu_monitor.h  |3 ++
 src/qemu/qemu_monitor_text.c |   71 ++
 src/qemu/qemu_monitor_text.h |4 ++
 5 files changed, 153 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index c1feb0f..6f1fc1a 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1407,6 +1407,40 @@ qemudExtractTTYPath(virConnectPtr conn,
 }
 
 static int
+qemudFindCharDevicePTYsMonitor(virConnectPtr conn,
+   virDomainObjPtr vm,
+   virHashTablePtr paths)
+{
+int i;
+
+#define LOOKUP_PTYS(array, arraylen, idprefix) \
+for (i = 0 ; i  (arraylen) ; i++) { \
+virDomainChrDefPtr chr = (array)[i]; \
+if (chr-type == VIR_DOMAIN_CHR_TYPE_PTY) { \
+char id[16]; \
+\
+if (snprintf(id, sizeof(id), idprefix %i, i) = sizeof(id)) \
+return -1; \
+\
+const char *path = (const char *) virHashLookup(paths, id); \
+if (path == NULL) { \
+qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, \
+ _(no assigned pty for device %s), id); \
+return -1; \
+} \
+\
+chr-data.file.path = strdup(path); \
+} \
+}
+
+LOOKUP_PTYS(vm-def-serials,   vm-def-nserials,   serial);
+LOOKUP_PTYS(vm-def-parallels, vm-def-nparallels, parallel);
+LOOKUP_PTYS(vm-def-channels,  vm-def-nchannels,  channel);
+
+return 0;
+}
+
+static int
 qemudFindCharDevicePTYs(virConnectPtr conn,
 virDomainObjPtr vm,
 const char *output,
@@ -1452,6 +1486,11 @@ qemudFindCharDevicePTYs(virConnectPtr conn,
 return 0;
 }
 
+static void qemudFreePtyPath(void *payload, const char *name ATTRIBUTE_UNUSED)
+{
+free(payload);
+}
+
 static int
 qemudWaitForMonitor(virConnectPtr conn,
 struct qemud_driver* driver,
@@ -1459,7 +1498,7 @@ qemudWaitForMonitor(virConnectPtr conn,
 {
 char buf[4096]; /* Plenty of space to get startup greeting */
 int logfd;
-int ret;
+int ret = -1;
 
 if ((logfd = qemudLogReadFD(conn, driver-logDir, vm-def-name, pos))
  0)
@@ -1485,7 +1524,32 @@ qemudWaitForMonitor(virConnectPtr conn,
 if (qemuConnectMonitor(vm)  0)
 return -1;
 
-return 0;
+/* Try to get the pty path mappings again via the monitor. This is much 
more
+ * reliable if it's available.
+ * Note that the monitor itself can be on a pty, so we still need to try 
the
+ * log output method. */
+virHashTablePtr paths = virHashCreate(0);
+if (paths == NULL) {
+virReportOOMError(NULL);
+goto cleanup;
+}
+
+qemuDomainObjEnterMonitor(vm);
+qemuDomainObjPrivatePtr priv = vm-privateData;
+ret = qemuMonitorGetPtyPaths(priv-mon, paths);
+qemuDomainObjExitMonitor(vm);
+
+VIR_DEBUG(qemuMonitorGetPtyPaths returned %i, ret);
+if (ret == 0) {
+ret = qemudFindCharDevicePTYsMonitor(conn, vm, paths);
+}
+
+cleanup:
+if (paths) {
+virHashFree(paths, qemudFreePtyPath);
+}
+
+return ret;
 }
 
 static int
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 103cf28..750e3e6 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -1267,3 +1267,12 @@ int qemuMonitorRemoveHostNetwork(qemuMonitorPtr mon,
 ret = qemuMonitorTextRemoveHostNetwork(mon, vlan, netname);
 return ret;
 }
+
+int qemuMonitorGetPtyPaths(qemuMonitorPtr mon,
+   virHashTablePtr paths)
+{
+DEBUG(mon=%p, fd=%d,
+  mon, mon-fd);
+
+return qemuMonitorTextGetPtyPaths(mon, paths);
+}
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index 8b1e3a3..db00b26 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -28,6 +28,7 @@
 #include internal.h
 
 #include domain_conf.h
+#include hash.h
 
 typedef struct _qemuMonitor qemuMonitor;
 typedef qemuMonitor *qemuMonitorPtr;
@@ -272,5 +273,7 @@ int

Re: [libvirt] [PATCH 3/3] Get QEMU pty paths from the monitor

2009-11-25 Thread Daniel P. Berrange 
On Mon, Nov 23, 2009 at 12:30:29PM +, Matthew Booth wrote:
 This change makes the QEMU driver get pty paths from the output of the monitor
 'info chardev' command. This output is structured, and contains both the name 
 of
 the device and the path on the same line. This is considerably more reliable
 than parsing the startup log output, which requires the parsing code to know
 which order QEMU will print pty information in.
 
 Note that we still need to parse the log output as the monitor itself may be 
 on
 a pty. This should be rare, however, and the new code will replace all pty 
 paths
 parsed by the log output method once the monitor is available.
 
 * src/qemu/qemu_monitor.(c|h) src/qemu_monitor_text.(c|h): Implement
   qemuMonitorGetPtyPaths().
 * src/qemu/qemu_driver.c: Get pty path information using
   qemuMonitorGetPtyPaths().
 ---
  src/qemu/qemu_driver.c   |   68 +++-
  src/qemu/qemu_monitor.c  |9 +
  src/qemu/qemu_monitor.h  |3 ++
  src/qemu/qemu_monitor_text.c |   71 
 ++
  src/qemu/qemu_monitor_text.h |4 ++
  5 files changed, 153 insertions(+), 2 deletions(-)
 
 diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
 index ebf44b0..90dd9cd 100644
 --- a/src/qemu/qemu_driver.c
 +++ b/src/qemu/qemu_driver.c
 @@ -1239,6 +1239,40 @@ qemudExtractTTYPath(virConnectPtr conn,
  }
  
  static int
 +qemudFindCharDevicePTYsMonitor(virConnectPtr conn,
 +   virDomainObjPtr vm,
 +   virHashTablePtr paths)
 +{
 +int i;
 +
 +#define LOOKUP_PTYS(array, arraylen, idprefix) \
 +for (i = 0 ; i  (arraylen) ; i++) { \
 +virDomainChrDefPtr chr = (array)[i]; \
 +if (chr-type == VIR_DOMAIN_CHR_TYPE_PTY) { \
 +char id[16]; \
 +\
 +if (snprintf(id, sizeof(id), idprefix %i, i) = sizeof(id)) \
 +return -1; \
 +\
 +const char *path = (const char *) virHashLookup(paths, id); \
 +if (path == NULL) { \
 +qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, \
 + _(no assigned pty for device %s), id); \
 +return -1; \
 +} \
 +\
 +chr-data.file.path = strdup(path); \
 +} \
 +}

Can you indent the  \  to they all line up in the right hand side. 

 +
 +LOOKUP_PTYS(vm-def-serials,   vm-def-nserials,   serial);
 +LOOKUP_PTYS(vm-def-parallels, vm-def-nparallels, parallel);
 +LOOKUP_PTYS(vm-def-channels,  vm-def-nchannels,  channel);
 +

#undef LOOKUP_PTYS

 +return 0;
 +}
 +
 +static int
  qemudFindCharDevicePTYs(virConnectPtr conn,
  virDomainObjPtr vm,
  const char *output,
 @@ -1284,6 +1318,11 @@ qemudFindCharDevicePTYs(virConnectPtr conn,
  return 0;
  }
  
 +static void qemudFreePtyPath(void *payload, const char *name 
 ATTRIBUTE_UNUSED)
 +{
 +free(payload);

VIR_FREE

 +}
 +
  static int
  qemudWaitForMonitor(virConnectPtr conn,
  struct qemud_driver* driver,
 @@ -1291,7 +1330,7 @@ qemudWaitForMonitor(virConnectPtr conn,
  {
  char buf[4096]; /* Plenty of space to get startup greeting */
  int logfd;
 -int ret;
 +int ret = -1;
  
  if ((logfd = qemudLogReadFD(conn, driver-logDir, vm-def-name, pos))
   0)
 @@ -1317,7 +1356,32 @@ qemudWaitForMonitor(virConnectPtr conn,
  if (qemuConnectMonitor(vm)  0)
  return -1;
  
 -return 0;
 +/* Try to get the pty path mappings again via the monitor. This is much 
 more
 + * reliable if it's available.
 + * Note that the monitor itself can be on a pty, so we still need to try 
 the
 + * log output method. */
 +virHashTablePtr paths = virHashCreate(0);
 +if (paths == NULL) {
 +virReportOOMError(NULL);
 +goto cleanup;
 +}
 +
 +qemuDomainObjEnterMonitor(vm);

This needs to be EnterMonitorWithDriver(driver, vm), since the 'driver'
is locked in this context

 +qemuDomainObjPrivatePtr priv = vm-privateData;
 +ret = qemuMonitorGetPtyPaths(priv-mon, paths);
 +qemuDomainObjExitMonitor(vm);

And ExitMonitorWithDriver

 +
 +VIR_DEBUG(qemuMonitorGetPtyPaths returned %i, ret);
 +if (ret == 0) {
 +ret = qemudFindCharDevicePTYsMonitor(conn, vm, paths);
 +}
 +
 +cleanup:
 +if (paths) {
 +virHashFree(paths, qemudFreePtyPath);
 +}
 +
 +return ret;
  }


 +
 +
 +/* Parse the output of info chardev and return a hash of pty paths.
 + *
 + * Output is:
 + * foo: filename=pty:/dev/pts/7
 + * monitor: filename=stdio
 + * serial0: filename=vc
 + * parallel0: filename=vc
 + *
 + * Non-pty lines are ignored. In the above example, key is 'foo', value is
 + * '/dev/pty/7'. The hash will contain only a single value.
 + */
 +
 +int qemuMonitorTextGetPtyPaths(qemuMonitorPtr mon,
 +   virHashTablePtr

[libvirt] [PATCH 3/3] Get QEMU pty paths from the monitor

2009-11-23 Thread Matthew Booth 
This change makes the QEMU driver get pty paths from the output of the monitor
'info chardev' command. This output is structured, and contains both the name of
the device and the path on the same line. This is considerably more reliable
than parsing the startup log output, which requires the parsing code to know
which order QEMU will print pty information in.

Note that we still need to parse the log output as the monitor itself may be on
a pty. This should be rare, however, and the new code will replace all pty paths
parsed by the log output method once the monitor is available.

* src/qemu/qemu_monitor.(c|h) src/qemu_monitor_text.(c|h): Implement
  qemuMonitorGetPtyPaths().
* src/qemu/qemu_driver.c: Get pty path information using
  qemuMonitorGetPtyPaths().
---
 src/qemu/qemu_driver.c   |   68 +++-
 src/qemu/qemu_monitor.c  |9 +
 src/qemu/qemu_monitor.h  |3 ++
 src/qemu/qemu_monitor_text.c |   71 ++
 src/qemu/qemu_monitor_text.h |4 ++
 5 files changed, 153 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index ebf44b0..90dd9cd 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1239,6 +1239,40 @@ qemudExtractTTYPath(virConnectPtr conn,
 }
 
 static int
+qemudFindCharDevicePTYsMonitor(virConnectPtr conn,
+   virDomainObjPtr vm,
+   virHashTablePtr paths)
+{
+int i;
+
+#define LOOKUP_PTYS(array, arraylen, idprefix) \
+for (i = 0 ; i  (arraylen) ; i++) { \
+virDomainChrDefPtr chr = (array)[i]; \
+if (chr-type == VIR_DOMAIN_CHR_TYPE_PTY) { \
+char id[16]; \
+\
+if (snprintf(id, sizeof(id), idprefix %i, i) = sizeof(id)) \
+return -1; \
+\
+const char *path = (const char *) virHashLookup(paths, id); \
+if (path == NULL) { \
+qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, \
+ _(no assigned pty for device %s), id); \
+return -1; \
+} \
+\
+chr-data.file.path = strdup(path); \
+} \
+}
+
+LOOKUP_PTYS(vm-def-serials,   vm-def-nserials,   serial);
+LOOKUP_PTYS(vm-def-parallels, vm-def-nparallels, parallel);
+LOOKUP_PTYS(vm-def-channels,  vm-def-nchannels,  channel);
+
+return 0;
+}
+
+static int
 qemudFindCharDevicePTYs(virConnectPtr conn,
 virDomainObjPtr vm,
 const char *output,
@@ -1284,6 +1318,11 @@ qemudFindCharDevicePTYs(virConnectPtr conn,
 return 0;
 }
 
+static void qemudFreePtyPath(void *payload, const char *name ATTRIBUTE_UNUSED)
+{
+free(payload);
+}
+
 static int
 qemudWaitForMonitor(virConnectPtr conn,
 struct qemud_driver* driver,
@@ -1291,7 +1330,7 @@ qemudWaitForMonitor(virConnectPtr conn,
 {
 char buf[4096]; /* Plenty of space to get startup greeting */
 int logfd;
-int ret;
+int ret = -1;
 
 if ((logfd = qemudLogReadFD(conn, driver-logDir, vm-def-name, pos))
  0)
@@ -1317,7 +1356,32 @@ qemudWaitForMonitor(virConnectPtr conn,
 if (qemuConnectMonitor(vm)  0)
 return -1;
 
-return 0;
+/* Try to get the pty path mappings again via the monitor. This is much 
more
+ * reliable if it's available.
+ * Note that the monitor itself can be on a pty, so we still need to try 
the
+ * log output method. */
+virHashTablePtr paths = virHashCreate(0);
+if (paths == NULL) {
+virReportOOMError(NULL);
+goto cleanup;
+}
+
+qemuDomainObjEnterMonitor(vm);
+qemuDomainObjPrivatePtr priv = vm-privateData;
+ret = qemuMonitorGetPtyPaths(priv-mon, paths);
+qemuDomainObjExitMonitor(vm);
+
+VIR_DEBUG(qemuMonitorGetPtyPaths returned %i, ret);
+if (ret == 0) {
+ret = qemudFindCharDevicePTYsMonitor(conn, vm, paths);
+}
+
+cleanup:
+if (paths) {
+virHashFree(paths, qemudFreePtyPath);
+}
+
+return ret;
 }
 
 static int
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index f0ef81b..92f005a 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -904,3 +904,12 @@ int qemuMonitorRemoveHostNetwork(qemuMonitorPtr mon,
 
 return qemuMonitorTextRemoveHostNetwork(mon, vlan, netname);
 }
+
+int qemuMonitorGetPtyPaths(qemuMonitorPtr mon,
+   virHashTablePtr paths)
+{
+DEBUG(mon=%p, fd=%d,
+  mon, mon-fd);
+
+return qemuMonitorTextGetPtyPaths(mon, paths);
+}
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index 71688cb..dc14bbe 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -28,6 +28,7 @@
 #include internal.h
 
 #include domain_conf.h
+#include hash.h
 
 typedef struct _qemuMonitor qemuMonitor;
 typedef qemuMonitor *qemuMonitorPtr;
@@ -244,5 +245,7 @@ int