Re: Virtual networking

[Mark McLoughlin <markmc@xxxxxxxxxx>]

Hi Karel,
	Thanks for the review ...

	Note, a lot of the code in the networking patches is just copied and
pasted from elsewhere in libvirt, so I'll fix up the original code
first.

On Tue, 2007-01-23 at 11:20 +0100, Karel Zak wrote:

> +    {"file", VSH_OT_DATA, VSH_OFLAG_REQ, gettext_noop("file containing an XML network description")},
> 
>  Cannot we use something like _N() rather than gettext_noop() ?

	That's one for Dan ... I suspect it's just because gettext_noop()
worked with xgettext, whereas _N() didn't. We'd need to pass
--keyword=_N to xgettext.

	(Also, it's always been N_() anywhere I've seen it)

>     names[ret++] = strdup(node->value);
>                    ^^^^^^^
>                where is free() for this string?
> 
> 
>  It seems like nice leak(s). Right?

	Yep, well spotted.

	I've appended the patch I committed.

Thanks,
Mark.

Index: ChangeLog
===================================================================
RCS file: /data/cvs/libvirt/ChangeLog,v
retrieving revision 1.319
diff -u -p -r1.319 ChangeLog
--- ChangeLog	22 Jan 2007 20:43:02 -0000	1.319
+++ ChangeLog	23 Jan 2007 12:28:16 -0000
@@ -0,0 +1,7 @@
+Mon Jan 23 12:28:42 IST 2007 Mark McLoughlin <markmc@xxxxxxxxxx>
+
+	Issues pointed out by Karel Zak <kzak@xxxxxxxxxx>
+
+	* src/virsh.c: fix up some syntax strings, use BUFSIZ
+	and free names returned from virConnectListDefinedDomains()
+	
Index: src/virsh.c
===================================================================
RCS file: /data/cvs/libvirt/src/virsh.c,v
retrieving revision 1.42
diff -u -p -r1.42 virsh.c
--- src/virsh.c	22 Jan 2007 20:43:02 -0000	1.42
+++ src/virsh.c	23 Jan 2007 12:28:16 -0000
@@ -309,7 +309,7 @@ cmdConnect(vshControl * ctl, vshCmd * cm
  * "list" command
  */
 static vshCmdInfo info_list[] = {
-    {"syntax", "list"},
+    {"syntax", "list [--inactive | --all]"},
     {"help", gettext_noop("list domains")},
     {"desc", gettext_noop("Returns list of domains.")},
     {NULL, NULL}
@@ -419,8 +419,10 @@ cmdList(vshControl * ctl, vshCmd * cmd A
         virDomainPtr dom = virDomainLookupByName(ctl->conn, names[i]);
 
         /* this kind of work with domains is not atomic operation */
-        if (!dom)
+        if (!dom) {
+	    free(names[i]);
             continue;
+	}
         ret = virDomainGetInfo(dom, &info);
 	id = virDomainGetID(dom);
 
@@ -439,6 +441,7 @@ cmdList(vshControl * ctl, vshCmd * cmd A
 	}
 
         virDomainFree(dom);
+	free(names[i]);
     }
     if (ids)
         free(ids);
@@ -546,7 +549,7 @@ cmdCreate(vshControl * ctl, vshCmd * cmd
     char *from;
     int found;
     int ret = TRUE;
-    char buffer[4096];
+    char buffer[BUFSIZ];
     int fd, l;
 
     if (!vshConnectionUsability(ctl, ctl->conn, TRUE))
@@ -601,7 +604,7 @@ cmdDefine(vshControl * ctl, vshCmd * cmd
     char *from;
     int found;
     int ret = TRUE;
-    char buffer[4096];
+    char buffer[BUFSIZ];
     int fd, l;
 
     if (!vshConnectionUsability(ctl, ctl->conn, TRUE))
@@ -677,7 +680,7 @@ cmdUndefine(vshControl * ctl, vshCmd * c
  * "start" command
  */
 static vshCmdInfo info_start[] = {
-    {"syntax", "start a domain "},
+    {"syntax", "start <domain>"},
     {"help", gettext_noop("start a (previously defined) inactive domain")},
     {"desc", gettext_noop("Start a domain.")},
     {NULL, NULL}