Re: [Qemu-devel] [PATCHv4] multiboot: copy the cmdline verbatim, unescape module strings

[Vlad Lungu]

Resubmitted with errors fixed.

Regards,

Vlad



On 12/21/2016 01:01 AM, no-re...@patchew.org wrote:
> Hi,
>
> Your series seems to have some coding style problems. See output below for
> more information:
>
> Subject: [Qemu-devel] [PATCHv4] multiboot: copy the cmdline verbatim, 
> unescape module strings
> Message-id: 1482140507-23607-1-git-send-email-vlad.lu...@windriver.com
> Type: series
>
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
>
> BASE=base
> n=1
> total=$(git log --oneline $BASE.. | wc -l)
> failed=0
>
> # Useful git options
> git config --local diff.renamelimit 0
> git config --local diff.renames True
>
> commits="$(git log --format=%H --reverse $BASE..)"
> for c in $commits; do
> echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
> if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; 
> then
> failed=1
> echo
> fi
> n=$((n+1))
> done
>
> exit $failed
> === TEST SCRIPT END ===
>
> Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
> Switched to a new branch 'test'
> 951c8be multiboot: copy the cmdline verbatim, unescape module strings
>
> === OUTPUT BEGIN ===
> Checking PATCH 1/1: multiboot: copy the cmdline verbatim, unescape module 
> strings...
> WARNING: line over 80 characters
> #42: FILE: hw/i386/multiboot.c:299:
> +next_initrd = (char *)get_opt_value(tmpbuf, sizeof(tmpbuf), 
> initrd_filename);
>
> ERROR: do not use assignment in if condition
> #50: FILE: hw/i386/multiboot.c:304:
> +if ((next_space = strchr(tmpbuf, ' ')))
>
> ERROR: braces {} are necessary for all arms of this statement
> #50: FILE: hw/i386/multiboot.c:304:
> +if ((next_space = strchr(tmpbuf, ' ')))
> [...]
>
> total: 2 errors, 1 warnings, 48 lines checked
>
> Your patch has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
>
> === OUTPUT END ===
>
> Test command exited with code: 1
>
>
> ---
> Email generated automatically by Patchew [http://patchew.org/].
> Please send your feedback to patchew-de...@freelists.org

Re: [Qemu-devel] [PATCHv4] multiboot: copy the cmdline verbatim, unescape module strings

[no-reply]

Hi,

Your series seems to have some coding style problems. See output below for
more information:

Subject: [Qemu-devel] [PATCHv4] multiboot: copy the cmdline verbatim, unescape 
module strings
Message-id: 1482140507-23607-1-git-send-email-vlad.lu...@windriver.com
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
951c8be multiboot: copy the cmdline verbatim, unescape module strings

=== OUTPUT BEGIN ===
Checking PATCH 1/1: multiboot: copy the cmdline verbatim, unescape module 
strings...
WARNING: line over 80 characters
#42: FILE: hw/i386/multiboot.c:299:
+next_initrd = (char *)get_opt_value(tmpbuf, sizeof(tmpbuf), 
initrd_filename);

ERROR: do not use assignment in if condition
#50: FILE: hw/i386/multiboot.c:304:
+if ((next_space = strchr(tmpbuf, ' ')))

ERROR: braces {} are necessary for all arms of this statement
#50: FILE: hw/i386/multiboot.c:304:
+if ((next_space = strchr(tmpbuf, ' ')))
[...]

total: 2 errors, 1 warnings, 48 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-de...@freelists.org

Re: [Qemu-devel] [PATCHv4] multiboot: copy the cmdline verbatim, unescape module strings

[Eduardo Habkost]

On Mon, Dec 19, 2016 at 11:41:47AM +0200, Vlad Lungu wrote:
> get_opt_value() truncates the value at the first comma
> Use memcpy() instead
> Unescape the module filename and parameters with get_opt_value()
> before calling mb_add_cmdline()
> 
> Signed-off-by: Vlad Lungu 

Reviewed-by: Eduardo Habkost 

-- 
Eduardo

[Qemu-devel] [PATCHv4] multiboot: copy the cmdline verbatim, unescape module strings

[Vlad Lungu]

get_opt_value() truncates the value at the first comma
Use memcpy() instead
Unescape the module filename and parameters with get_opt_value()
before calling mb_add_cmdline()

Signed-off-by: Vlad Lungu 
---
 hw/i386/multiboot.c | 19 +--
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
index 387caa6..efe11ae 100644
--- a/hw/i386/multiboot.c
+++ b/hw/i386/multiboot.c
@@ -109,7 +109,7 @@ static uint32_t mb_add_cmdline(MultibootState *s, const 
char *cmdline)
 hwaddr p = s->offset_cmdlines;
 char *b = (char *)s->mb_buf + p;
 
-get_opt_value(b, strlen(cmdline) + 1, cmdline);
+memcpy(b, cmdline, strlen(cmdline) + 1);
 s->offset_cmdlines += strlen(b) + 1;
 return s->mb_buf_phys + p;
 }
@@ -287,7 +287,7 @@ int load_multiboot(FWCfgState *fw_cfg,
 mbs.offset_bootloader = mbs.offset_cmdlines + cmdline_len;
 
 if (initrd_filename) {
-char *next_initrd, not_last;
+char *next_initrd, not_last, tmpbuf[strlen(initrd_filename) + 1];
 
 mbs.offset_mods = mbs.mb_buf_size;
 
@@ -296,25 +296,24 @@ int load_multiboot(FWCfgState *fw_cfg,
 int mb_mod_length;
 uint32_t offs = mbs.mb_buf_size;
 
-next_initrd = (char *)get_opt_value(NULL, 0, initrd_filename);
+next_initrd = (char *)get_opt_value(tmpbuf, sizeof(tmpbuf), 
initrd_filename);
 not_last = *next_initrd;
-*next_initrd = '\0';
 /* if a space comes after the module filename, treat everything
after that as parameters */
-hwaddr c = mb_add_cmdline(, initrd_filename);
-if ((next_space = strchr(initrd_filename, ' ')))
+hwaddr c = mb_add_cmdline(, tmpbuf);
+if ((next_space = strchr(tmpbuf, ' ')))
 *next_space = '\0';
-mb_debug("multiboot loading module: %s\n", initrd_filename);
-mb_mod_length = get_image_size(initrd_filename);
+mb_debug("multiboot loading module: %s\n", tmpbuf);
+mb_mod_length = get_image_size(tmpbuf);
 if (mb_mod_length < 0) {
-fprintf(stderr, "Failed to open file '%s'\n", initrd_filename);
+fprintf(stderr, "Failed to open file '%s'\n", tmpbuf);
 exit(1);
 }
 
 mbs.mb_buf_size = TARGET_PAGE_ALIGN(mb_mod_length + 
mbs.mb_buf_size);
 mbs.mb_buf = g_realloc(mbs.mb_buf, mbs.mb_buf_size);
 
-load_image(initrd_filename, (unsigned char *)mbs.mb_buf + offs);
+load_image(tmpbuf, (unsigned char *)mbs.mb_buf + offs);
 mb_add_mod(, mbs.mb_buf_phys + offs,
mbs.mb_buf_phys + offs + mb_mod_length, c);
 
-- 
1.9.1