[Qemu-devel] [patch] replace all strdup() with g_strdup()

[Qemu-devel] [patch] replace all strdup() with g_strdup()

[Jun Koi]

[-- Attachment #1: Type: text/plain, Size: 740 bytes --]

This patch replaces all the strdup() with g_strdup()

Signed-off-by: Jun Koi <junkoi2004@gmail.com>

$ diffstat strdup.diff
 cmd.c               |    4 ++--
 envlist.c           |    6 +++---
 exec.c              |    2 +-
 hw/isa-bus.c        |    2 +-
 hw/pc.c             |    2 +-
 hw/pci.c            |    2 +-
 hw/qdev.c           |    2 +-
 hw/scsi-bus.c       |    2 +-
 hw/sun4m.c          |    6 +++---
 hw/sun4u.c          |    2 +-
 hw/usb-msd.c        |    2 +-
 hw/virtio-blk.c     |    2 +-
 ia64-dis.c          |    2 +-
 microblaze-dis.c    |   18 +++++++++---------
 path.c              |    2 +-
 readline.c          |    2 +-
 test-qmp-commands.c |   12 ++++++------
 17 files changed, 35 insertions(+), 35 deletions(-)

[-- Attachment #2: strdup.diff --]
[-- Type: application/octet-stream, Size: 12240 bytes --]

diff --git a/cmd.c b/cmd.c
index 0806e18..33c315e 100644
--- a/cmd.c
+++ b/cmd.c
@@ -160,7 +160,7 @@ void command_loop(void)
     const cmdinfo_t *ct;
 
     for (i = 0; !done && i < ncmdline; i++) {
-        input = strdup(cmdline[i]);
+        input = g_strdup(cmdline[i]);
         if (!input) {
             fprintf(stderr, _("cannot strdup command '%s': %s\n"),
                     cmdline[i], strerror(errno));
@@ -274,7 +274,7 @@ fetchline(void)
 		el_set(el, EL_PROMPT, el_get_prompt);
 		el_set(el, EL_HIST, history, (const char *)hist);
 	}
-	line = strdup(el_gets(el, &count));
+	line = g_strdup(el_gets(el, &count));
 	if (line) {
 		if (count > 0)
 			line[count-1] = '\0';
diff --git a/envlist.c b/envlist.c
index f2303cd..70986b6 100644
--- a/envlist.c
+++ b/envlist.c
@@ -109,7 +109,7 @@ envlist_parse(envlist_t *envlist, const char *env,
 	 * We need to make temporary copy of the env string
 	 * as strtok_r(3) modifies it while it tokenizes.
 	 */
-	if ((tmpenv = strdup(env)) == NULL)
+	if ((tmpenv = g_strdup(env)) == NULL)
 		return (errno);
 
 	envvar = strtok_r(tmpenv, ",", &envsave);
@@ -167,7 +167,7 @@ envlist_setenv(envlist_t *envlist, const char *env)
 
 	if ((entry = malloc(sizeof (*entry))) == NULL)
 		return (errno);
-	if ((entry->ev_var = strdup(env)) == NULL) {
+	if ((entry->ev_var = g_strdup(env)) == NULL) {
 		free(entry);
 		return (errno);
 	}
@@ -235,7 +235,7 @@ envlist_to_environ(const envlist_t *envlist, size_t *count)
 
 	for (entry = envlist->el_entries.lh_first; entry != NULL;
 	    entry = entry->ev_link.le_next) {
-		*(penv++) = strdup(entry->ev_var);
+		*(penv++) = g_strdup(entry->ev_var);
 	}
 	*penv = NULL; /* NULL terminate the list */
 
diff --git a/exec.c b/exec.c
index 06889bd..c9c0a93 100644
--- a/exec.c
+++ b/exec.c
@@ -1618,7 +1618,7 @@ void cpu_set_log(int log_flags)
 
 void cpu_set_log_filename(const char *filename)
 {
-    logfilename = strdup(filename);
+    logfilename = g_strdup(filename);
     if (logfile) {
         fclose(logfile);
         logfile = NULL;
diff --git a/hw/isa-bus.c b/hw/isa-bus.c
index 7c2c261..27582d3 100644
--- a/hw/isa-bus.c
+++ b/hw/isa-bus.c
@@ -205,7 +205,7 @@ static char *isabus_get_fw_dev_path(DeviceState *dev)
         snprintf(path + off, sizeof(path) - off, "@%04x", d->ioport_id);
     }
 
-    return strdup(path);
+    return g_strdup(path);
 }
 
 MemoryRegion *isa_address_space(ISADevice *dev)
diff --git a/hw/pc.c b/hw/pc.c
index 03466ec..1807d2d 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -738,7 +738,7 @@ static void load_linux(void *fw_cfg,
     fw_cfg_add_i32(fw_cfg, FW_CFG_CMDLINE_ADDR, cmdline_addr);
     fw_cfg_add_i32(fw_cfg, FW_CFG_CMDLINE_SIZE, strlen(kernel_cmdline)+1);
     fw_cfg_add_bytes(fw_cfg, FW_CFG_CMDLINE_DATA,
-                     (uint8_t*)strdup(kernel_cmdline),
+                     (uint8_t*)g_strdup(kernel_cmdline),
                      strlen(kernel_cmdline)+1);
 
     if (protocol >= 0x202) {
diff --git a/hw/pci.c b/hw/pci.c
index 399227f..bebfcd0 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -1941,7 +1941,7 @@ static char *pcibus_get_fw_dev_path(DeviceState *dev)
                    PCI_SLOT(d->devfn));
     if (PCI_FUNC(d->devfn))
         snprintf(path + off, sizeof(path) + off, ",%x", PCI_FUNC(d->devfn));
-    return strdup(path);
+    return g_strdup(path);
 }
 
 static char *pcibus_get_dev_path(DeviceState *dev)
diff --git a/hw/qdev.c b/hw/qdev.c
index 83913c7..1d0fa9a 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -1030,7 +1030,7 @@ char* qdev_get_fw_dev_path(DeviceState *dev)
 
     path[l-1] = '\0';
 
-    return strdup(path);
+    return g_strdup(path);
 }
 
 char *qdev_get_type(DeviceState *dev, Error **errp)
diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index 64e709e..7a16f5c 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -1373,7 +1373,7 @@ static char *scsibus_get_fw_dev_path(DeviceState *dev)
     snprintf(path, sizeof(path), "channel@%x/%s@%x,%x", d->channel,
              qdev_fw_name(dev), d->id, d->lun);
 
-    return strdup(path);
+    return g_strdup(path);
 }
 
 SCSIDevice *scsi_device_find(SCSIBus *bus, int channel, int id, int lun)
diff --git a/hw/sun4m.c b/hw/sun4m.c
index 3f172ad..4f57428 100644
--- a/hw/sun4m.c
+++ b/hw/sun4m.c
@@ -992,7 +992,7 @@ static void sun4m_hw_init(const struct sun4m_hwdef *hwdef, ram_addr_t RAM_size,
         fw_cfg_add_i32(fw_cfg, FW_CFG_KERNEL_CMDLINE, CMDLINE_ADDR);
         pstrcpy_targphys("cmdline", CMDLINE_ADDR, TARGET_PAGE_SIZE, kernel_cmdline);
         fw_cfg_add_bytes(fw_cfg, FW_CFG_CMDLINE_DATA,
-                         (uint8_t*)strdup(kernel_cmdline),
+                         (uint8_t*)g_strdup(kernel_cmdline),
                          strlen(kernel_cmdline) + 1);
         fw_cfg_add_i32(fw_cfg, FW_CFG_CMDLINE_SIZE,
                        strlen(kernel_cmdline) + 1);
@@ -1603,7 +1603,7 @@ static void sun4d_hw_init(const struct sun4d_hwdef *hwdef, ram_addr_t RAM_size,
         fw_cfg_add_i32(fw_cfg, FW_CFG_KERNEL_CMDLINE, CMDLINE_ADDR);
         pstrcpy_targphys("cmdline", CMDLINE_ADDR, TARGET_PAGE_SIZE, kernel_cmdline);
         fw_cfg_add_bytes(fw_cfg, FW_CFG_CMDLINE_DATA,
-                         (uint8_t*)strdup(kernel_cmdline),
+                         (uint8_t*)g_strdup(kernel_cmdline),
                          strlen(kernel_cmdline) + 1);
     } else {
         fw_cfg_add_i32(fw_cfg, FW_CFG_KERNEL_CMDLINE, 0);
@@ -1795,7 +1795,7 @@ static void sun4c_hw_init(const struct sun4c_hwdef *hwdef, ram_addr_t RAM_size,
         fw_cfg_add_i32(fw_cfg, FW_CFG_KERNEL_CMDLINE, CMDLINE_ADDR);
         pstrcpy_targphys("cmdline", CMDLINE_ADDR, TARGET_PAGE_SIZE, kernel_cmdline);
         fw_cfg_add_bytes(fw_cfg, FW_CFG_CMDLINE_DATA,
-                         (uint8_t*)strdup(kernel_cmdline),
+                         (uint8_t*)g_strdup(kernel_cmdline),
                          strlen(kernel_cmdline) + 1);
     } else {
         fw_cfg_add_i32(fw_cfg, FW_CFG_KERNEL_CMDLINE, 0);
diff --git a/hw/sun4u.c b/hw/sun4u.c
index 97600a9..9c3f0df 100644
--- a/hw/sun4u.c
+++ b/hw/sun4u.c
@@ -821,7 +821,7 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
         fw_cfg_add_i32(fw_cfg, FW_CFG_CMDLINE_SIZE,
                        strlen(kernel_cmdline) + 1);
         fw_cfg_add_bytes(fw_cfg, FW_CFG_CMDLINE_DATA,
-                         (uint8_t*)strdup(kernel_cmdline),
+                         (uint8_t*)g_strdup(kernel_cmdline),
                          strlen(kernel_cmdline) + 1);
     } else {
         fw_cfg_add_i32(fw_cfg, FW_CFG_CMDLINE_SIZE, 0);
diff --git a/hw/usb-msd.c b/hw/usb-msd.c
index 4c06950..55d1926 100644
--- a/hw/usb-msd.c
+++ b/hw/usb-msd.c
@@ -537,7 +537,7 @@ static int usb_msd_initfn(USBDevice *dev)
         /* try to fall back to value set with legacy -drive serial=... */
         dinfo = drive_get_by_blockdev(bs);
         if (*dinfo->serial) {
-            s->serial = strdup(dinfo->serial);
+            s->serial = g_strdup(dinfo->serial);
         }
     }
     if (s->serial) {
diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index 4b0d113..fcd0663 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -592,7 +592,7 @@ VirtIODevice *virtio_blk_init(DeviceState *dev, BlockConf *conf,
         /* try to fall back to value set with legacy -drive serial=... */
         dinfo = drive_get_by_blockdev(conf->bs);
         if (*dinfo->serial) {
-            *serial = strdup(dinfo->serial);
+            *serial = g_strdup(dinfo->serial);
         }
     }
 
diff --git a/ia64-dis.c b/ia64-dis.c
index 2a103e6..0420b7b 100644
--- a/ia64-dis.c
+++ b/ia64-dis.c
@@ -10271,7 +10271,7 @@ make_ia64_opcode (ia64_insn opcode, const char *name, int place, int depind)
 {
   struct ia64_opcode *res =
     (struct ia64_opcode *) malloc (sizeof (struct ia64_opcode));
-  res->name = strdup (name);
+  res->name = g_strdup (name);
   res->type = main_table[place].opcode_type;
   res->num_outputs = main_table[place].num_outputs;
   res->opcode = opcode;
diff --git a/microblaze-dis.c b/microblaze-dis.c
index 16c312f..91415a2 100644
--- a/microblaze-dis.c
+++ b/microblaze-dis.c
@@ -630,7 +630,7 @@ get_field (long instr, long mask, unsigned short low)
 {
   char tmpstr[25];
   sprintf(tmpstr, "%s%d", register_prefix, (int)((instr & mask) >> low));
-  return(strdup(tmpstr));
+  return(g_strdup(tmpstr));
 }
 
 static char *
@@ -638,7 +638,7 @@ get_field_imm (long instr)
 {
   char tmpstr[25];
   sprintf(tmpstr, "%d", (short)((instr & IMM_MASK) >> IMM_LOW));
-  return(strdup(tmpstr));
+  return(g_strdup(tmpstr));
 }
 
 static char *
@@ -646,7 +646,7 @@ get_field_imm5 (long instr)
 {
   char tmpstr[25];
   sprintf(tmpstr, "%d", (short)((instr & IMM5_MASK) >> IMM_LOW));
-  return(strdup(tmpstr));
+  return(g_strdup(tmpstr));
 }
 
 static char *
@@ -654,7 +654,7 @@ get_field_rfsl (long instr)
 {
   char tmpstr[25];
   sprintf(tmpstr, "%s%d", fsl_register_prefix, (short)((instr & RFSL_MASK) >> IMM_LOW));
-  return(strdup(tmpstr));
+  return(g_strdup(tmpstr));
 }
 
 static char *
@@ -662,7 +662,7 @@ get_field_imm15 (long instr)
 {
   char tmpstr[25];
   sprintf(tmpstr, "%d", (short)((instr & IMM15_MASK) >> IMM_LOW));
-  return(strdup(tmpstr));
+  return(g_strdup(tmpstr));
 }
 
 #if 0
@@ -671,7 +671,7 @@ get_field_unsigned_imm (long instr)
 {
   char tmpstr[25];
   sprintf(tmpstr, "%d", (int)((instr & IMM_MASK) >> IMM_LOW));
-  return(strdup(tmpstr));
+  return(g_strdup(tmpstr));
 }
 #endif
 
@@ -684,7 +684,7 @@ get_field_unsigned_imm (long instr)
   
   sprintf(tmpstr, "%s%s", register_prefix, (((instr & IMM_MASK) >> IMM_LOW) & REG_MSR_MASK) == 0 ? "pc" : "msr");
   
-  return(strdup(tmpstr));
+  return(g_strdup(tmpstr));
   }
 */
 
@@ -739,7 +739,7 @@ get_field_special (long instr, struct op_code_struct * op)
      {
        if ( ((((instr & IMM_MASK) >> IMM_LOW) ^ op->immval_mask) & 0xE000) == REG_PVR_MASK) {
 	 sprintf(tmpstr, "%spvr%d", register_prefix, (unsigned short)(((instr & IMM_MASK) >> IMM_LOW) ^ op->immval_mask) ^ REG_PVR_MASK);
-	 return(strdup(tmpstr));
+	 return(g_strdup(tmpstr));
        } else {
 	 strcpy(spr, "pc");
        }
@@ -748,7 +748,7 @@ get_field_special (long instr, struct op_code_struct * op)
    }
    
    sprintf(tmpstr, "%s%s", register_prefix, spr);
-   return(strdup(tmpstr));
+   return(g_strdup(tmpstr));
 }
 
 unsigned long
diff --git a/path.c b/path.c
index ef3f277..f7fd81e 100644
--- a/path.c
+++ b/path.c
@@ -46,7 +46,7 @@ static struct pathelem *new_entry(const char *root,
                                   const char *name)
 {
     struct pathelem *new = malloc(sizeof(*new));
-    new->name = strdup(name);
+    new->name = g_strdup(name);
     if (asprintf(&new->pathname, "%s/%s", root, name) == -1) {
         printf("Cannot allocate memory\n");
         exit(1);
diff --git a/readline.c b/readline.c
index a6c0039..c72efad 100644
--- a/readline.c
+++ b/readline.c
@@ -254,7 +254,7 @@ static void readline_hist_add(ReadLineState *rs, const char *cmdline)
 	idx = READLINE_MAX_CMDS - 1;
     }
     if (new_entry == NULL)
-	new_entry = strdup(cmdline);
+	new_entry = g_strdup(cmdline);
     rs->history[idx] = new_entry;
     rs->hist_entry = -1;
 }
diff --git a/test-qmp-commands.c b/test-qmp-commands.c
index fa5a7bd..7489f84 100644
--- a/test-qmp-commands.c
+++ b/test-qmp-commands.c
@@ -18,19 +18,19 @@ UserDefTwo * qmp_user_def_cmd2(UserDefOne * ud1a, UserDefOne * ud1b, Error **err
     UserDefOne *ud1c = g_malloc0(sizeof(UserDefOne));
     UserDefOne *ud1d = g_malloc0(sizeof(UserDefOne));
 
-    ud1c->string = strdup(ud1a->string);
+    ud1c->string = g_strdup(ud1a->string);
     ud1c->integer = ud1a->integer;
-    ud1d->string = strdup(ud1b->string);
+    ud1d->string = g_strdup(ud1b->string);
     ud1d->integer = ud1b->integer;
 
     ret = g_malloc0(sizeof(UserDefTwo));
-    ret->string = strdup("blah1");
-    ret->dict.string = strdup("blah2");
+    ret->string = g_strdup("blah1");
+    ret->dict.string = g_strdup("blah2");
     ret->dict.dict.userdef = ud1c;
-    ret->dict.dict.string = strdup("blah3");
+    ret->dict.dict.string = g_strdup("blah3");
     ret->dict.has_dict2 = true;
     ret->dict.dict2.userdef = ud1d;
-    ret->dict.dict2.string = strdup("blah4");
+    ret->dict.dict2.string = g_strdup("blah4");
 
     return ret;
 }

Re: [Qemu-devel] [patch] replace all strdup() with g_strdup()

[陳韋任]

Hi,

  You can check http://wiki.qemu.org/Contribute/SubmitAPatch to see how to send
patch to qemu-devel. :)

Regards,
chenwj

-- 
Wei-Ren Chen (陳韋任)
Computer Systems Lab, Institute of Information Science,
Academia Sinica, Taiwan (R.O.C.)
Tel:886-2-2788-3799 #1667
Homepage: http://people.cs.nctu.edu.tw/~chenwj

Re: [Qemu-devel] [patch] replace all strdup() with g_strdup()

[Peter Maydell]

On 16 December 2011 08:07, Jun Koi <junkoi2004@gmail.com> wrote:
> This patch replaces all the strdup() with g_strdup()

I don't think you can do this as a pure search-and-replace.
For example this change:
> --- a/envlist.c
> +++ b/envlist.c
> @@ -109,7 +109,7 @@ envlist_parse(envlist_t *envlist, const char *env,
>  	 * We need to make temporary copy of the env string
>  	 * as strtok_r(3) modifies it while it tokenizes.
>  	 */
> -	if ((tmpenv = strdup(env)) == NULL)
> +	if ((tmpenv = g_strdup(env)) == NULL)
> 		return (errno);
>
> 	envvar = strtok_r(tmpenv, ",", &envsave);

means we're allocating memory with g_strdup but freeing it with
plain free rather than g_free.

You have to take each usage of memory, look for all the places which
allocate/free/strdup it, and change them all at once.

-- PMM

Re: [Qemu-devel] [patch] replace all strdup() with g_strdup()

[Paolo Bonzini]

On 12/16/2011 09:07 AM, Jun Koi wrote:
> This patch replaces all the strdup() with g_strdup()
>
> Signed-off-by: Jun Koi<junkoi2004@gmail.com>
>
> $ diffstat strdup.diff
>   cmd.c               |    4 ++--
>   envlist.c           |    6 +++---
>   exec.c              |    2 +-
>   hw/isa-bus.c        |    2 +-
>   hw/pc.c             |    2 +-
>   hw/pci.c            |    2 +-
>   hw/qdev.c           |    2 +-
>   hw/scsi-bus.c       |    2 +-
>   hw/sun4m.c          |    6 +++---
>   hw/sun4u.c          |    2 +-
>   hw/usb-msd.c        |    2 +-
>   hw/virtio-blk.c     |    2 +-
>   ia64-dis.c          |    2 +-
>   microblaze-dis.c    |   18 +++++++++---------
>   path.c              |    2 +-
>   readline.c          |    2 +-
>   test-qmp-commands.c |   12 ++++++------
>   17 files changed, 35 insertions(+), 35 deletions(-)

You cannot do this unless you also convert all free to g_free.  So you 
also cannot do this when you are passing strings to libraries that take 
ownership of the strings.  A search-and-replace conversion is not possible.

Paolo

Re: [Qemu-devel] [patch] replace all strdup() with g_strdup()

[Daniel P. Berrange]

On Fri, Dec 16, 2011 at 10:26:33AM +0100, Paolo Bonzini wrote:
> On 12/16/2011 09:07 AM, Jun Koi wrote:
> >This patch replaces all the strdup() with g_strdup()
> >
> >Signed-off-by: Jun Koi<junkoi2004@gmail.com>
> >
> >$ diffstat strdup.diff
> >  cmd.c               |    4 ++--
> >  envlist.c           |    6 +++---
> >  exec.c              |    2 +-
> >  hw/isa-bus.c        |    2 +-
> >  hw/pc.c             |    2 +-
> >  hw/pci.c            |    2 +-
> >  hw/qdev.c           |    2 +-
> >  hw/scsi-bus.c       |    2 +-
> >  hw/sun4m.c          |    6 +++---
> >  hw/sun4u.c          |    2 +-
> >  hw/usb-msd.c        |    2 +-
> >  hw/virtio-blk.c     |    2 +-
> >  ia64-dis.c          |    2 +-
> >  microblaze-dis.c    |   18 +++++++++---------
> >  path.c              |    2 +-
> >  readline.c          |    2 +-
> >  test-qmp-commands.c |   12 ++++++------
> >  17 files changed, 35 insertions(+), 35 deletions(-)
> 
> You cannot do this unless you also convert all free to g_free.  So
> you also cannot do this when you are passing strings to libraries
> that take ownership of the strings.  A search-and-replace conversion
> is not possible.

Yes & no. In general you are correct that g_malloc/g_strdup needs to
be matched with g_free, but in the context of the QEMU binary at least
we don't strictly need that.

The general issue is that GLib's memory allocators default to the
system malloc/free, but with    g_mem_set_vtable   it is possible
to override those allocators.

So any libraries using GLib should definitely take care to match
g_malloc/g_strdup/g_free, but if you are a self contained program
that never calls g_mem_set_vtable, we don't technically have to
worry about it.

That all said, we probably *do* want to take care todo the matching
for any parts of the QEMU codebase which may end up being built into
ELF libraries for external consumption.

Regards,
Daniel.
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [patch] replace all strdup() with g_strdup()

[Paolo Bonzini]

On 12/16/2011 10:41 AM, Daniel P. Berrange wrote:
> Yes&  no. In general you are correct that g_malloc/g_strdup needs to
> be matched with g_free, but in the context of the QEMU binary at least
> we don't strictly need that.
>
> The general issue is that GLib's memory allocators default to the
> system malloc/free, but with    g_mem_set_vtable   it is possible
> to override those allocators.
>
> So any libraries using GLib should definitely take care to match
> g_malloc/g_strdup/g_free, but if you are a self contained program
> that never calls g_mem_set_vtable, we don't technically have to
> worry about it.

I think the keyword here is "technically". :)  If we want to use the 
GLib profiling allocators or any other kind of statistic gathering, we 
do have to match the allocations.  Right now, we're not getting it 100% 
right, but sweeping conversions make it harder to just grep-w for 
malloc/free/strdup.

Paolo