[Qemu-devel] [PATCH][Outreachy] Signed-off-by: Sarah Khan <sarahjmi07@gmail.com>

[Qemu-devel] [PATCH][Outreachy] Signed-off-by: Sarah Khan <sarahjmi07@gmail.com>

[Sarah Khan]

This patch replaces malloc() with g_malloc() as stated to be done in bitesized task

diff --git a/thunk.c b/thunk.c
index f057d86..bddabae 100644
--- a/thunk.c
+++ b/thunk.c
@@ -88,7 +88,7 @@ void thunk_register_struct(int id, const char *name, const argtype *types)
     for(i = 0;i < 2; i++) {
         offset = 0;
         max_align = 1;
-        se->field_offsets[i] = malloc(nb_fields * sizeof(int));
+        se->field_offsets[i] = g_malloc(nb_fields * sizeof(int));
         type_ptr = se->field_types;
         for(j = 0;j < nb_fields; j++) {
             size = thunk_type_size(type_ptr, i);
---
 thunk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/thunk.c b/thunk.c
index f057d86..bddabae 100644
--- a/thunk.c
+++ b/thunk.c
@@ -88,7 +88,7 @@ void thunk_register_struct(int id, const char *name, const argtype *types)
     for(i = 0;i < 2; i++) {
         offset = 0;
         max_align = 1;
-        se->field_offsets[i] = malloc(nb_fields * sizeof(int));
+        se->field_offsets[i] = g_malloc(nb_fields * sizeof(int));
         type_ptr = se->field_types;
         for(j = 0;j < nb_fields; j++) {
             size = thunk_type_size(type_ptr, i);
-- 
1.9.1

Re: [Qemu-devel] [PATCH][Outreachy] Signed-off-by: Sarah Khan <sarahjmi07@gmail.com>

[Eric Blake]

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

On 03/04/2016 07:53 AM, Sarah Khan wrote:
> This patch replaces malloc() with g_malloc() as stated to be done in bitesized task
> 
> diff --git a/thunk.c b/thunk.c
> index f057d86..bddabae 100644
> --- a/thunk.c
> +++ b/thunk.c
> @@ -88,7 +88,7 @@ void thunk_register_struct(int id, const char *name, const argtype *types)
>      for(i = 0;i < 2; i++) {
>          offset = 0;
>          max_align = 1;
> -        se->field_offsets[i] = malloc(nb_fields * sizeof(int));
> +        se->field_offsets[i] = g_malloc(nb_fields * sizeof(int));
>          type_ptr = se->field_types;
>          for(j = 0;j < nb_fields; j++) {
>              size = thunk_type_size(type_ptr, i);

Where is the corresponding free()?  g_malloc() must be paired with
g_free(), so you need to convert both places at once.

Also, your patch is missing a Signed-off-by designation; without that,
we can't accept it.  More hints at:
http://wiki.qemu.org/Contribute/SubmitAPatch
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=f6f94e2ab1#n297

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH][Outreachy] Signed-off-by: Sarah Khan <sarahjmi07@gmail.com>

[Eric Blake]

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

On 03/04/2016 08:44 AM, Eric Blake wrote:
> On 03/04/2016 07:53 AM, Sarah Khan wrote:
>> This patch replaces malloc() with g_malloc() as stated to be done in bitesized task
>>
>> diff --git a/thunk.c b/thunk.c
>> index f057d86..bddabae 100644
>> --- a/thunk.c
>> +++ b/thunk.c
>> @@ -88,7 +88,7 @@ void thunk_register_struct(int id, const char *name, const argtype *types)
>>      for(i = 0;i < 2; i++) {
>>          offset = 0;
>>          max_align = 1;
>> -        se->field_offsets[i] = malloc(nb_fields * sizeof(int));
>> +        se->field_offsets[i] = g_malloc(nb_fields * sizeof(int));
>>          type_ptr = se->field_types;
>>          for(j = 0;j < nb_fields; j++) {
>>              size = thunk_type_size(type_ptr, i);
> 
> Where is the corresponding free()?  g_malloc() must be paired with
> g_free(), so you need to convert both places at once.
> 
> Also, your patch is missing a Signed-off-by designation; without that,
> we can't accept it.

Oh, I see you DID include a S-o-b, but in the subject line of the patch
instead of the commit body.  Which still needs work.  You need a
one-line summary as the subject, not your S-o-b, so the commit message
should look more like:

thunk: Replace malloc with g_malloc()

This replacement was suggested as part of the bite-sized tasks.

Signed-off-by: Sarah Khan <sarahjmi07@gmail.com>


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH][Outreachy] Signed-off-by: Sarah Khan <sarahjmi07@gmail.com>

[Eric Blake]

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

On 03/04/2016 08:49 AM, Eric Blake wrote:

> Oh, I see you DID include a S-o-b, but in the subject line of the patch
> instead of the commit body.  Which still needs work.  You need a
> one-line summary as the subject, not your S-o-b, so the commit message
> should look more like:
> 
> thunk: Replace malloc with g_malloc()
> 
> This replacement was suggested as part of the bite-sized tasks.
> 
> Signed-off-by: Sarah Khan <sarahjmi07@gmail.com>

Oh, and I'd be remiss if I did not add:

This looks like your first contribution.  Welcome to the QEMU community,
and hope you enjoy it here!  Keep in mind that email is a bad medium for
expressing emotion, and that reviewers tend to point out flaws without
looking for the good; but in reality, we are grateful when someone steps
up to help with the bite-sized tasks.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH][Outreachy] Signed-off-by: Sarah Khan <sarahjmi07@gmail.com>

[Peter Maydell]

On 4 March 2016 at 15:44, Eric Blake <eblake@redhat.com> wrote:
> On 03/04/2016 07:53 AM, Sarah Khan wrote:
>> This patch replaces malloc() with g_malloc() as stated to be done in bitesized task
>>
>> diff --git a/thunk.c b/thunk.c
>> index f057d86..bddabae 100644
>> --- a/thunk.c
>> +++ b/thunk.c
>> @@ -88,7 +88,7 @@ void thunk_register_struct(int id, const char *name, const argtype *types)
>>      for(i = 0;i < 2; i++) {
>>          offset = 0;
>>          max_align = 1;
>> -        se->field_offsets[i] = malloc(nb_fields * sizeof(int));
>> +        se->field_offsets[i] = g_malloc(nb_fields * sizeof(int));
>>          type_ptr = se->field_types;
>>          for(j = 0;j < nb_fields; j++) {
>>              size = thunk_type_size(type_ptr, i);
>
> Where is the corresponding free()?  g_malloc() must be paired with
> g_free(), so you need to convert both places at once.

There is no corresponding free(). thunk_register_struct() is called
only at startup from the linux-user code in order to populate the
struct_entries array; this data structure then remains live for
the entire lifetime of the program and is automatically freed when
QEMU exits.

This is worth mentioning in the commit message, but the code is
correct I think:
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH][Outreachy] Signed-off-by: Sarah Khan <sarahjmi07@gmail.com>

[Sarah Khan]

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

Ok. Will complete the required.

Thanks,
Sarah
On 4 March 2016 at 15:44, Eric Blake <eblake@redhat.com> wrote:
> On 03/04/2016 07:53 AM, Sarah Khan wrote:
>> This patch replaces malloc() with g_malloc() as stated to be done in
bitesized task
>>
>> diff --git a/thunk.c b/thunk.c
>> index f057d86..bddabae 100644
>> --- a/thunk.c
>> +++ b/thunk.c
>> @@ -88,7 +88,7 @@ void thunk_register_struct(int id, const char *name,
const argtype *types)
>>      for(i = 0;i < 2; i++) {
>>          offset = 0;
>>          max_align = 1;
>> -        se->field_offsets[i] = malloc(nb_fields * sizeof(int));
>> +        se->field_offsets[i] = g_malloc(nb_fields * sizeof(int));
>>          type_ptr = se->field_types;
>>          for(j = 0;j < nb_fields; j++) {
>>              size = thunk_type_size(type_ptr, i);
>
> Where is the corresponding free()?  g_malloc() must be paired with
> g_free(), so you need to convert both places at once.

There is no corresponding free(). thunk_register_struct() is called
only at startup from the linux-user code in order to populate the
struct_entries array; this data structure then remains live for
the entire lifetime of the program and is automatically freed when
QEMU exits.

This is worth mentioning in the commit message, but the code is
correct I think:
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[-- Attachment #2: Type: text/html, Size: 1941 bytes --]

Re: [Qemu-devel] [PATCH][Outreachy] Signed-off-by: Sarah Khan <sarahjmi07@gmail.com>

[Eric Blake]

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

On 03/04/2016 09:06 AM, Peter Maydell wrote:

>>> +++ b/thunk.c
>>> @@ -88,7 +88,7 @@ void thunk_register_struct(int id, const char *name, const argtype *types)
>>>      for(i = 0;i < 2; i++) {
>>>          offset = 0;
>>>          max_align = 1;
>>> -        se->field_offsets[i] = malloc(nb_fields * sizeof(int));
>>> +        se->field_offsets[i] = g_malloc(nb_fields * sizeof(int));
>>>          type_ptr = se->field_types;
>>>          for(j = 0;j < nb_fields; j++) {
>>>              size = thunk_type_size(type_ptr, i);
>>
>> Where is the corresponding free()?  g_malloc() must be paired with
>> g_free(), so you need to convert both places at once.
> 
> There is no corresponding free(). thunk_register_struct() is called
> only at startup from the linux-user code in order to populate the
> struct_entries array; this data structure then remains live for
> the entire lifetime of the program and is automatically freed when
> QEMU exits.

Fair enough.  However, g_new(int, nb_fields) is probably a bit nicer
than g_malloc() (in that it would detect multiplication overflow if
nb_fields were ever oversized).


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH][Outreachy] Signed-off-by: Sarah Khan <sarahjmi07@gmail.com>

[Peter Maydell]

On 4 March 2016 at 16:35, Eric Blake <eblake@redhat.com> wrote:
> On 03/04/2016 09:06 AM, Peter Maydell wrote:
>
>>>> +++ b/thunk.c
>>>> @@ -88,7 +88,7 @@ void thunk_register_struct(int id, const char *name, const argtype *types)
>>>>      for(i = 0;i < 2; i++) {
>>>>          offset = 0;
>>>>          max_align = 1;
>>>> -        se->field_offsets[i] = malloc(nb_fields * sizeof(int));
>>>> +        se->field_offsets[i] = g_malloc(nb_fields * sizeof(int));
>>>>          type_ptr = se->field_types;
>>>>          for(j = 0;j < nb_fields; j++) {
>>>>              size = thunk_type_size(type_ptr, i);
>>>
>>> Where is the corresponding free()?  g_malloc() must be paired with
>>> g_free(), so you need to convert both places at once.
>>
>> There is no corresponding free(). thunk_register_struct() is called
>> only at startup from the linux-user code in order to populate the
>> struct_entries array; this data structure then remains live for
>> the entire lifetime of the program and is automatically freed when
>> QEMU exits.
>
> Fair enough.  However, g_new(int, nb_fields) is probably a bit nicer
> than g_malloc() (in that it would detect multiplication overflow if
> nb_fields were ever oversized).

Yes, good idea.

thanks
-- PMM

[Qemu-devel] [PATCH v2][Outreachy Round 12]

[Sarah Khan]

Replaced g_malloc() with g_new() as it would detect multiplication overflow if nb_fields ever oversize.

There is no corresponding free(). thunk_register_struct() is called
only at startup from the linux-user code in order to populate the
struct_entries array; this data structure then remains live for
the entire lifetime of the program and is automatically freed when
QEMU exits.

This replacement was suggested as part of the bite-sized tasks.

Signed-off-by: Sarah Khan <sarahjmi07@gmail.com>
---
Changes in v2 :Make commit message clearer
	       Replace g_malloc() with g_new()	
---
 thunk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/thunk.c b/thunk.c
index bddabae..6237702 100644
--- a/thunk.c
+++ b/thunk.c
@@ -88,7 +88,7 @@ void thunk_register_struct(int id, const char *name, const argtype *types)
     for(i = 0;i < 2; i++) {
         offset = 0;
         max_align = 1;
-        se->field_offsets[i] = g_malloc(nb_fields * sizeof(int));
+        se->field_offsets[i] = g_new(int,nb_fields);
         type_ptr = se->field_types;
         for(j = 0;j < nb_fields; j++) {
             size = thunk_type_size(type_ptr, i);
-- 
1.9.1

Re: [Qemu-devel] [PATCH v2][Outreachy Round 12]

[Markus Armbruster]

Your commit message isn't quite right, yet.  The first line is empty,
which leads to

    Subject: [PATCH v2][Outreachy Round 12]

in e-mail, which isn't really useful.  It should be a line of the form

    subsystem: Headline describing the patch briefly

In e-mail, this becomes something like

    Subject: [PATCH v2] subsystem: Headline describing the patch briefly

The [PATCH v2] gets inserted by git-format-patch.

Finding the appropriate subsystem is unfortunately less than
straightforward.  You can use "git log --oneline FILENAME..." for ideas.
For your patch, I'd use linux-user.

Since this is a rather busy list, it's important to cc the right people
on patches.  Start with "scripts/get_maintainer PATCH-FILE".  The script
looks up the files you patch in MAINTAINERS for you.  In your case, its
output is

    Riku Voipio <riku.voipio@iki.fi> (maintainer:Overall)
    qemu-devel@nongnu.org (open list:All patches CC here)

Send the patch to qemu-devel, cc: Riku.

All of this and much more is documented at
<http://wiki.qemu.org/Contribute/SubmitAPatch>.  It's a learning curve,
but we'll gladly help you along.

Sarah Khan <sarahjmi07@gmail.com> writes:

> Replaced g_malloc() with g_new() as it would detect multiplication overflow if nb_fields ever oversize.

Long line, please break it like you do in the next paragraph.

> There is no corresponding free(). thunk_register_struct() is called
> only at startup from the linux-user code in order to populate the
> struct_entries array; this data structure then remains live for
> the entire lifetime of the program and is automatically freed when
> QEMU exits.
>
> This replacement was suggested as part of the bite-sized tasks.
>
> Signed-off-by: Sarah Khan <sarahjmi07@gmail.com>
> ---
> Changes in v2 :Make commit message clearer
> 	       Replace g_malloc() with g_new()	
> ---
>  thunk.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/thunk.c b/thunk.c
> index bddabae..6237702 100644
> --- a/thunk.c
> +++ b/thunk.c
> @@ -88,7 +88,7 @@ void thunk_register_struct(int id, const char *name, const argtype *types)
>      for(i = 0;i < 2; i++) {
>          offset = 0;
>          max_align = 1;
> -        se->field_offsets[i] = g_malloc(nb_fields * sizeof(int));
> +        se->field_offsets[i] = g_new(int,nb_fields);
>          type_ptr = se->field_types;
>          for(j = 0;j < nb_fields; j++) {
>              size = thunk_type_size(type_ptr, i);

Oh, this is an *incremental* patch: it applies on top of your v1.
That's awkward.  Your v2 should *replace* v1, not add to it.

Style nitpick: we want a space after comma.  Recommend to check your
patches with scripts/checkpatch.pl.  Output for this one is:

    ERROR: space required after that ',' (ctx:VxV)
    #127: FILE: thunk.c:91:
    +        se->field_offsets[i] = g_new(int,nb_fields);
                                             ^

    total: 1 errors, 0 warnings, 8 lines checked

    /home/armbru/Mail/mail/redhat/xlst/qemu-devel/385421 has style problems, please review.  If any of these errors
    are false positives report them to the maintainer, see
    CHECKPATCH in MAINTAINERS.

Welcome to qemu-devel, Sarah!

Re: [Qemu-devel] [PATCH v2][Outreachy Round 12]

[Sarah Khan]

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

Thank you for your feedback.
Will do the required.
Sorry for the delay as I was out of station.


Thanks,
Sarah

On Sat, Mar 5, 2016 at 4:12 PM, Markus Armbruster <armbru@redhat.com> wrote:

> Your commit message isn't quite right, yet.  The first line is empty,
> which leads to
>
>     Subject: [PATCH v2][Outreachy Round 12]
>
> in e-mail, which isn't really useful.  It should be a line of the form
>
>     subsystem: Headline describing the patch briefly
>
> In e-mail, this becomes something like
>
>     Subject: [PATCH v2] subsystem: Headline describing the patch briefly
>
> The [PATCH v2] gets inserted by git-format-patch.
>
> Finding the appropriate subsystem is unfortunately less than
> straightforward.  You can use "git log --oneline FILENAME..." for ideas.
> For your patch, I'd use linux-user.
>
> Since this is a rather busy list, it's important to cc the right people
> on patches.  Start with "scripts/get_maintainer PATCH-FILE".  The script
> looks up the files you patch in MAINTAINERS for you.  In your case, its
> output is
>
>     Riku Voipio <riku.voipio@iki.fi> (maintainer:Overall)
>     qemu-devel@nongnu.org (open list:All patches CC here)
>
> Send the patch to qemu-devel, cc: Riku.
>
> All of this and much more is documented at
> <http://wiki.qemu.org/Contribute/SubmitAPatch>.  It's a learning curve,
> but we'll gladly help you along.
>
> Sarah Khan <sarahjmi07@gmail.com> writes:
>
> > Replaced g_malloc() with g_new() as it would detect multiplication
> overflow if nb_fields ever oversize.
>
> Long line, please break it like you do in the next paragraph.
>
> > There is no corresponding free(). thunk_register_struct() is called
> > only at startup from the linux-user code in order to populate the
> > struct_entries array; this data structure then remains live for
> > the entire lifetime of the program and is automatically freed when
> > QEMU exits.
> >
> > This replacement was suggested as part of the bite-sized tasks.
> >
> > Signed-off-by: Sarah Khan <sarahjmi07@gmail.com>
> > ---
> > Changes in v2 :Make commit message clearer
> >              Replace g_malloc() with g_new()
> > ---
> >  thunk.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/thunk.c b/thunk.c
> > index bddabae..6237702 100644
> > --- a/thunk.c
> > +++ b/thunk.c
> > @@ -88,7 +88,7 @@ void thunk_register_struct(int id, const char *name,
> const argtype *types)
> >      for(i = 0;i < 2; i++) {
> >          offset = 0;
> >          max_align = 1;
> > -        se->field_offsets[i] = g_malloc(nb_fields * sizeof(int));
> > +        se->field_offsets[i] = g_new(int,nb_fields);
> >          type_ptr = se->field_types;
> >          for(j = 0;j < nb_fields; j++) {
> >              size = thunk_type_size(type_ptr, i);
>
> Oh, this is an *incremental* patch: it applies on top of your v1.
> That's awkward.  Your v2 should *replace* v1, not add to it.
>
> Style nitpick: we want a space after comma.  Recommend to check your
> patches with scripts/checkpatch.pl.  Output for this one is:
>
>     ERROR: space required after that ',' (ctx:VxV)
>     #127: FILE: thunk.c:91:
>     +        se->field_offsets[i] = g_new(int,nb_fields);
>                                              ^
>
>     total: 1 errors, 0 warnings, 8 lines checked
>
>     /home/armbru/Mail/mail/redhat/xlst/qemu-devel/385421 has style
> problems, please review.  If any of these errors
>     are false positives report them to the maintainer, see
>     CHECKPATCH in MAINTAINERS.
>
> Welcome to qemu-devel, Sarah!
>

[-- Attachment #2: Type: text/html, Size: 5031 bytes --]