Re: [Qemu-devel] [PATCH v02] add 1394 bus support

[Itamar Tal <itamar@guardicore.com>]

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

On Sun, Apr 19, 2015 at 8:21 PM, Andreas Färber <afaerber@suse.de> 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 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=127.0.0.1,port=13940,nodelay,id=fw0 -device
ohci-1394,chardev=fw0

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ärber" <afaerber@suse.de
> > <mailto:afaerber@suse.de>> wrote:
> >
> >     Am 19.04.2015 um 12:52 schrieb itamar.tal4@gmail.com
> >     <mailto:itamar.tal4@gmail.com>:
> >     > From: Itamar Tal <itamar@guardicore.com
> >     <mailto:itamar@guardicore.com>>
> >     >
> >     > ---
> >
> >     Still no Signed-off-by...
> >
> >     Regards,
> >     Andreas
> >
> >     --
> >     SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> >     GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
> >     Graham Norton; HRB 21284 (AG Nürnberg)
> >
>
>
> --
> SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
> Graham Norton; HRB 21284 (AG Nürnberg)
>

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