From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44503) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YkAMB-0005qY-Lw for qemu-devel@nongnu.org; Mon, 20 Apr 2015 07:59:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YkAM8-0007ah-GA for qemu-devel@nongnu.org; Mon, 20 Apr 2015 07:59:19 -0400 Received: from mail-ie0-x22c.google.com ([2607:f8b0:4001:c03::22c]:35983) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YkAM8-0007ZB-1u for qemu-devel@nongnu.org; Mon, 20 Apr 2015 07:59:16 -0400 Received: by iebrs15 with SMTP id rs15so114065845ieb.3 for ; Mon, 20 Apr 2015 04:59:14 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <5533E437.70108@suse.de> References: <1429440741-48575-1-git-send-email-itamar@guardicore.com> <1429440741-48575-2-git-send-email-itamar@guardicore.com> <5533CE0D.4020807@suse.de> <5533E437.70108@suse.de> Date: Mon, 20 Apr 2015 14:59:14 +0300 Message-ID: From: Itamar Tal Content-Type: multipart/alternative; boundary=bcaec5196a2dac1c1e051426a8eb Subject: Re: [Qemu-devel] [PATCH v02] add 1394 bus support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?Q?Andreas_F=C3=A4rber?= Cc: thuth@redhat.com, Ariel Zeitlin , Itamar Tal , qemu-devel@nongnu.org --bcaec5196a2dac1c1e051426a8eb Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On Sun, Apr 19, 2015 at 8:21 PM, Andreas F=C3=A4rber wro= te: > Hi, > > Am 19.04.2015 um 18:38 schrieb Itamar Tal: > > I've set it just after the subject field in the patch? Should I add it > > somewhere else and resubmit? > > Please compare other QEMU or Linux patches: It needs to go before --- > into the commit message. git commit --amend -s will place it correctly. > OK - I'll do that > > BTW I'm missing a type_init() and type_register_static() in the bottom > of hcd-ohci.c and wonder how you managed to test it without. > I test it adding the following commands the command line -chardev socket,host=3D127.0.0.1,port=3D13940,nodelay,id=3Dfw0 -device ohci-1394,chardev=3Dfw0 which creates a new ohci-1394 device which I can work with and connect between machines. > > Did you copy this code from some other codebase? For one, hcd-ohci.c has > "address@hidden", which looks like you copied it from some mailing list > archive (then you need to replicate those authors' Signed-off-bys), and > for another the Coding Style does not exactly match that of QEMU, in > particular you are using a lot of custom _t types whereas QEMU uses > CamelCase type names. BTW if you consistently used typedefs for your > unions, you could avoid those macros declaring phy etc. unions inline, no= ? > - I've copied the patch from the last reply I've sent to qemu-dev so the address is hidden. - I ran checkpatch.pl to align the code with the QEMU code before submitting (no errors/warnings). Other than the XXX_t types, anything else? I'll fix it, no problem > > Please also drop the empty last line in the new files you add. Did you > run scripts/checkpatch.pl? Running git-am on a clean branch can also > help highlight whitespace errors. > - Did checkpatch.pl. I'll also try git-am. > > hcd-ohci.h is missing a license/copyright header. > - OK, I'll add it > > hcd_pci_exit() has commented-out code - please fix or remove. > - no problem > hcd_pci_init() has the opening brace placed wrong. > - no problem > > It looks like a generic PCI device, so why are you placing the config > option into i386 and x86_64 configs only rather than pci.mak? > - I've followed the suggestion by made by Thomas Huth, what do you suggest? When trying to build it with other architectures, sometimes it worked but many times I was missing some qemu dependencies APIs. > > Regards, > Andreas > > P.S. Please don't top-post. > - gotcha > > > On Apr 19, 2015 6:47 PM, "Andreas F=C3=A4rber" > > wrote: > > > > Am 19.04.2015 um 12:52 schrieb itamar.tal4@gmail.com > > : > > > From: Itamar Tal > > > > > > > > --- > > > > Still no Signed-off-by... > > > > Regards, > > Andreas > > > > -- > > SUSE Linux GmbH, Maxfeldstr. 5, 90409 N=C3=BCrnberg, Germany > > GF: Felix Imend=C3=B6rffer, Jane Smithard, Jennifer Guild, Dilip Up= manyu, > > Graham Norton; HRB 21284 (AG N=C3=BCrnberg) > > > > > -- > SUSE Linux GmbH, Maxfeldstr. 5, 90409 N=C3=BCrnberg, Germany > GF: Felix Imend=C3=B6rffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu, > Graham Norton; HRB 21284 (AG N=C3=BCrnberg) > --bcaec5196a2dac1c1e051426a8eb Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable


On Sun, Apr 19, 2015 at 8:21 PM, Andreas F=C3=A4rber = <afaerber@suse.de<= /a>> wrote:
Hi,

Am 19.04.2015 um 18:38 schrieb Itamar Tal:
> I've set it just after the subject field in the patch? Should I ad= d it
> somewhere else and resubmit?

Please compare other QEMU or Linux patches: It needs to go before --= -
into the commit message. git commit --amend -s will place it correctly.
=

OK - I'll do that=C2=A0

BTW I'm missing a type_init() and type_register_static() in the bottom<= br> of hcd-ohci.c and wonder how you managed to test it without.
=C2=A0I test it adding the following commands the command line
=
-chardev socket,host=3D127.0.0.1,port=3D13940,nodelay,id=3Dfw0 -device= ohci-1394,chardev=3Dfw0

which creates a new o= hci-1394 device which I can work with and connect between machines.

Did you copy this code from some other codebase? For one, hcd-ohci.c has "address@hidden", which looks like you copied it from some mailin= g list
archive (then you need to replicate those authors' Signed-off-bys), and=
for another the Coding Style does not exactly match that of QEMU, in
particular you are using a lot of custom _t types whereas QEMU uses
CamelCase type names. BTW if you consistently used typedefs for your
unions, you could avoid those macros declaring phy etc. unions inline, no?<= br>

- I've copied the patch from the la= st reply I've sent to qemu-dev so the address is hidden.
- I = ran checkpatch.pl to align the code wi= th the QEMU code before submitting (no errors/warnings). Other than the XXX= _t types, anything else? I'll fix it, no problem

Please also drop the empty last line in the new files you add. Did you
run scripts/checkpatch.p= l? Running git-am on a clean branch can also
help highlight whitespace errors.
- Did checkpatch.pl. I'll also try = git-am.

hcd-ohci.h is missing a license/copyright header.
- OK= , I'll add it=C2=A0

hcd_pci_exit() has commented-out code - please fix or remove.
- no problem
hcd_pci_init() has the opening brace placed wrong.
- n= o problem

It looks like a generic PCI device, so why are you placing the config
option into i386 and x86_64 configs only rather than pci.mak?
- I've followed the suggestion by made by=C2=A0Thomas Huth, what do you su= ggest? When trying to build it with other architectures, sometimes it worke= d but many times I was missing some qemu=C2=A0dependencies=C2=A0APIs.

Regards,
Andreas

P.S. Please don't top-post.
- gotcha=C2=A0

> On Apr 19, 2015 6:47 PM, "Andreas F=C3=A4rber" <afaerber@suse.de
> <mailto:afaerber@suse.de>> wrote:
>
>=C2=A0 =C2=A0 =C2=A0Am 19.04.2015 um 12:52 schrieb itamar.tal4@gmail.com
>=C2=A0 =C2=A0 =C2=A0<mailto:itamar.tal4@gmail.com>:
>=C2=A0 =C2=A0 =C2=A0> From: Itamar Tal <itamar@guardicore.com
>=C2=A0 =C2=A0 =C2=A0<mailto:itamar@guardicore.com>>
>=C2=A0 =C2=A0 =C2=A0>
>=C2=A0 =C2=A0 =C2=A0> ---
>
>=C2=A0 =C2=A0 =C2=A0Still no Signed-off-by...
>
>=C2=A0 =C2=A0 =C2=A0Regards,
>=C2=A0 =C2=A0 =C2=A0Andreas
>
>=C2=A0 =C2=A0 =C2=A0--
>=C2=A0 =C2=A0 =C2=A0SUSE Linux GmbH, Maxfeldstr. 5, 90409 N=C3=BCrnberg= , Germany
>=C2=A0 =C2=A0 =C2=A0GF: Felix Imend=C3=B6rffer, Jane Smithard, Jennifer= Guild, Dilip Upmanyu,
>=C2=A0 =C2=A0 =C2=A0Graham Norton; HRB 21284 (AG N=C3=BCrnberg)
>


--
SUSE Linux GmbH, Maxfeldstr. 5, 90409 N=C3=BCrnberg, Germany
GF: Felix Imend=C3=B6rffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu, Graham Norton; HRB 21284 (AG N=C3=BCrnberg)

--bcaec5196a2dac1c1e051426a8eb--