diff mbox

[4/6] monitor: Remove unused monitor_print_filename

Message ID 150e59818a7f8a0c5928f9f08d31c2248029ab57.1394579440.git.crobinso@redhat.com
State New
Headers show

Commit Message

Cole Robinson March 11, 2014, 11:15 p.m. UTC 
Cc: Luiz Capitulino <lcapitulino@redhat.com>
Cc: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
---
 include/monitor/monitor.h  |  1 -
 monitor.c                  | 27 ---------------------------
 stubs/Makefile.objs        |  1 -
 stubs/mon-print-filename.c |  6 ------
 4 files changed, 35 deletions(-)
 delete mode 100644 stubs/mon-print-filename.c

Comments

Andreas Färber March 22, 2014, 8:06 p.m. UTC | #1 
Am 12.03.2014 00:15, schrieb Cole Robinson:
> Cc: Luiz Capitulino <lcapitulino@redhat.com>
> Cc: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Cole Robinson <crobinso@redhat.com>

Given that it even had its own stub file, the commit message should
please explain why it is no longer needed and thus okay to drop.

Regards,
Andreas
Cole Robinson March 22, 2014, 9:44 p.m. UTC | #2 
On 03/22/2014 04:06 PM, Andreas Färber wrote:
> Am 12.03.2014 00:15, schrieb Cole Robinson:
>> Cc: Luiz Capitulino <lcapitulino@redhat.com>
>> Cc: Markus Armbruster <armbru@redhat.com>
>> Signed-off-by: Cole Robinson <crobinso@redhat.com>
> 
> Given that it even had its own stub file, the commit message should
> please explain why it is no longer needed and thus okay to drop.
> 
> Regards,
> Andreas
> 

The last usage of it was dropped with:

commit fbe2e26c15af35e4d157874dc80f6a19eebaa83b
Author: Kevin Wolf <kwolf@redhat.com>
Date:   Wed Jun 19 16:10:55 2013 +0200

    hmp: Make "info block" output more readable

Previously it was used to escape file= and backing_file= in 'info block' output.

I sent this patch because I noticed it was unused, maybe someone else can
comment on whether it's still useful?

- Cole
Luiz Capitulino March 24, 2014, 1:09 p.m. UTC | #3 
On Sat, 22 Mar 2014 21:06:41 +0100
Andreas Färber <afaerber@suse.de> wrote:

> Am 12.03.2014 00:15, schrieb Cole Robinson:
> > Cc: Luiz Capitulino <lcapitulino@redhat.com>
> > Cc: Markus Armbruster <armbru@redhat.com>
> > Signed-off-by: Cole Robinson <crobinso@redhat.com>
> 
> Given that it even had its own stub file, the commit message should
> please explain why it is no longer needed and thus okay to drop.

Because it's unused?
Andreas Färber March 24, 2014, 1:46 p.m. UTC | #4 
Am 24.03.2014 14:09, schrieb Luiz Capitulino:
> On Sat, 22 Mar 2014 21:06:41 +0100
> Andreas Färber <afaerber@suse.de> wrote:
> 
>> Am 12.03.2014 00:15, schrieb Cole Robinson:
>>> Cc: Luiz Capitulino <lcapitulino@redhat.com>
>>> Cc: Markus Armbruster <armbru@redhat.com>
>>> Signed-off-by: Cole Robinson <crobinso@redhat.com>
>>
>> Given that it even had its own stub file, the commit message should
>> please explain why it is no longer needed and thus okay to drop.
> 
> Because it's unused?

That's no real answer. Why did it need to be escaped before the commit
Cole pointed to and now no longer is?

My point was that someone went throught the hassle of creating a stub
file just for this functionality, so it's not some random unused
function in some file. (But I'm not particularly attached to it, just
cautioning about removing too much in this non-verbose way, making it
hard to follow for outsiders.)

Andreas
Markus Armbruster March 26, 2014, 1:46 p.m. UTC | #5 
Andreas Färber <afaerber@suse.de> writes:

> Am 24.03.2014 14:09, schrieb Luiz Capitulino:
>> On Sat, 22 Mar 2014 21:06:41 +0100
>> Andreas Färber <afaerber@suse.de> wrote:
>> 
>>> Am 12.03.2014 00:15, schrieb Cole Robinson:
>>>> Cc: Luiz Capitulino <lcapitulino@redhat.com>
>>>> Cc: Markus Armbruster <armbru@redhat.com>
>>>> Signed-off-by: Cole Robinson <crobinso@redhat.com>
>>>
>>> Given that it even had its own stub file, the commit message should
>>> please explain why it is no longer needed and thus okay to drop.
>> 
>> Because it's unused?
>
> That's no real answer. Why did it need to be escaped before the commit
> Cole pointed to and now no longer is?

Valid question, but it applies to the commits that dropped uses, not to
Cole's patch.

The function goes back to 2006, commit fef3074 "Escape filname printout
properly".  This made "info block" print some but not all funny
characters in filenames more legibly.  Dropped in Kevin's commit fbe2e26
"hmp: Make "info block" output more readable" from last June.  Could be
regarded as regression.  Not sure we care.

A second use was introduced a couple of months later, in commit a9ce859
"info vnc command".  Reason for escaping isn't obvious to me.  Dropped
in commit 1ff7df1 "Enhance 'info vnc' monitor output", which prints
something else entirely.

> My point was that someone went throught the hassle of creating a stub
> file just for this functionality, so it's not some random unused
> function in some file. (But I'm not particularly attached to it, just
> cautioning about removing too much in this non-verbose way, making it
> hard to follow for outsiders.)

Sounds like you'd like a more vebose commit message.  What information
would you like to see?
diff mbox

Patch

diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index a49ea11..42d8671 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -79,7 +79,6 @@  int monitor_handle_fd_param(Monitor *mon, const char *fdname);
 void monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)
     GCC_FMT_ATTR(2, 0);
 void monitor_printf(Monitor *mon, const char *fmt, ...) GCC_FMT_ATTR(2, 3);
-void monitor_print_filename(Monitor *mon, const char *filename);
 void monitor_flush(Monitor *mon);
 int monitor_set_cpu(int cpu_index);
 int monitor_get_cpu_index(void);
diff --git a/monitor.c b/monitor.c
index 342e83b..c8428a1 100644
--- a/monitor.c
+++ b/monitor.c
@@ -352,33 +352,6 @@  void monitor_printf(Monitor *mon, const char *fmt, ...)
     va_end(ap);
 }
 
-void monitor_print_filename(Monitor *mon, const char *filename)
-{
-    int i;
-
-    for (i = 0; filename[i]; i++) {
-        switch (filename[i]) {
-        case ' ':
-        case '"':
-        case '\\':
-            monitor_printf(mon, "\\%c", filename[i]);
-            break;
-        case '\t':
-            monitor_printf(mon, "\\t");
-            break;
-        case '\r':
-            monitor_printf(mon, "\\r");
-            break;
-        case '\n':
-            monitor_printf(mon, "\\n");
-            break;
-        default:
-            monitor_printf(mon, "%c", filename[i]);
-            break;
-        }
-    }
-}
-
 static int GCC_FMT_ATTR(2, 3) monitor_fprintf(FILE *stream,
                                               const char *fmt, ...)
 {
diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index df3aa7a..94ded0c 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -14,7 +14,6 @@  stub-obj-y += iothread-lock.o
 stub-obj-y += migr-blocker.o
 stub-obj-y += mon-is-qmp.o
 stub-obj-y += mon-printf.o
-stub-obj-y += mon-print-filename.o
 stub-obj-y += mon-protocol-event.o
 stub-obj-y += mon-set-error.o
 stub-obj-y += pci-drive-hot-add.o
diff --git a/stubs/mon-print-filename.c b/stubs/mon-print-filename.c
deleted file mode 100644
index 9c93964..0000000
--- a/stubs/mon-print-filename.c
+++ /dev/null
@@ -1,6 +0,0 @@ 
-#include "qemu-common.h"
-#include "monitor/monitor.h"
-
-void monitor_print_filename(Monitor *mon, const char *filename)
-{
-}