| Message ID | 1499803333-9052-2-git-send-email-mark.cave-ayland@ilande.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jul 11, 2017 at 09:02:11PM +0100, Mark Cave-Ayland wrote: > This will enable the fw_cfg device to be placed anywhere within the QOM tree > regardless of its machine location. > > Note that we also add a comment to document the behaviour that we return NULL to > indicate failure where either no fw_cfg device or multiple fw_cfg devices are > found. > > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > --- > hw/nvram/fw_cfg.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c > index 99bdbc2..8ef889a 100644 > --- a/hw/nvram/fw_cfg.c > +++ b/hw/nvram/fw_cfg.c > @@ -1017,7 +1017,12 @@ FWCfgState *fw_cfg_init_mem(hwaddr ctl_addr, hwaddr data_addr) > > FWCfgState *fw_cfg_find(void) > { > - return FW_CFG(object_resolve_path(FW_CFG_PATH, NULL)); > + /* Returns FWCfgState if only one fw_cfg device type exists. If zero or > + more than one fw_cfg device are found then NULL is returned as per the > + object_resolve_path_type() documentation. This behaviour is correct as > + it ensures that we detect both missing fw_cfg devices and multiple > + fw_cfg devices which could result in unpredictable behaviour. */ > + return FW_CFG(object_resolve_path_type("", TYPE_FW_CFG, NULL)); > } I believe a one-line "returns NULL unless there is exactly one fw_cfg device" (similar to the one at find_vmgenid_dev()) would suffice. :)
On 11/07/17 21:10, Eduardo Habkost wrote: > On Tue, Jul 11, 2017 at 09:02:11PM +0100, Mark Cave-Ayland wrote: >> This will enable the fw_cfg device to be placed anywhere within the QOM tree >> regardless of its machine location. >> >> Note that we also add a comment to document the behaviour that we return NULL to >> indicate failure where either no fw_cfg device or multiple fw_cfg devices are >> found. >> >> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >> --- >> hw/nvram/fw_cfg.c | 7 ++++++- >> 1 file changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c >> index 99bdbc2..8ef889a 100644 >> --- a/hw/nvram/fw_cfg.c >> +++ b/hw/nvram/fw_cfg.c >> @@ -1017,7 +1017,12 @@ FWCfgState *fw_cfg_init_mem(hwaddr ctl_addr, hwaddr data_addr) >> >> FWCfgState *fw_cfg_find(void) >> { >> - return FW_CFG(object_resolve_path(FW_CFG_PATH, NULL)); >> + /* Returns FWCfgState if only one fw_cfg device type exists. If zero or >> + more than one fw_cfg device are found then NULL is returned as per the >> + object_resolve_path_type() documentation. This behaviour is correct as >> + it ensures that we detect both missing fw_cfg devices and multiple >> + fw_cfg devices which could result in unpredictable behaviour. */ >> + return FW_CFG(object_resolve_path_type("", TYPE_FW_CFG, NULL)); >> } > > I believe a one-line "returns NULL unless there is exactly one > fw_cfg device" (similar to the one at find_vmgenid_dev()) would > suffice. :) I got the impression from the thread that Igor thought the semantics of fw_cfg_find() weren't clear enough, so wanted to make sure a good explanation was included in the patch. I'll wait to see if there's any further feedback before a v9... ATB, Mark.
On Tue, Jul 11, 2017 at 05:10:59PM -0300, Eduardo Habkost wrote: > On Tue, Jul 11, 2017 at 09:02:11PM +0100, Mark Cave-Ayland wrote: > > This will enable the fw_cfg device to be placed anywhere within the QOM tree > > regardless of its machine location. > > > > Note that we also add a comment to document the behaviour that we return NULL to > > indicate failure where either no fw_cfg device or multiple fw_cfg devices are > > found. > > > > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > > --- > > hw/nvram/fw_cfg.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c > > index 99bdbc2..8ef889a 100644 > > --- a/hw/nvram/fw_cfg.c > > +++ b/hw/nvram/fw_cfg.c > > @@ -1017,7 +1017,12 @@ FWCfgState *fw_cfg_init_mem(hwaddr ctl_addr, hwaddr data_addr) > > > > FWCfgState *fw_cfg_find(void) > > { > > - return FW_CFG(object_resolve_path(FW_CFG_PATH, NULL)); > > + /* Returns FWCfgState if only one fw_cfg device type exists. If zero or > > + more than one fw_cfg device are found then NULL is returned as per the > > + object_resolve_path_type() documentation. This behaviour is correct as > > + it ensures that we detect both missing fw_cfg devices and multiple > > + fw_cfg devices which could result in unpredictable behaviour. */ > > + return FW_CFG(object_resolve_path_type("", TYPE_FW_CFG, NULL)); > > } > > I believe a one-line "returns NULL unless there is exactly one > fw_cfg device" (similar to the one at find_vmgenid_dev()) would > suffice. :) Multi-line comments also should formatted /* * like * this */ not /* like * this */ > -- > Eduardo
On 11/07/17 21:18, Michael S. Tsirkin wrote: > Multi-line comments also should formatted > > /* > * like > * this > */ > > not > > /* like > * this */ > Interesting, I never knew there was a preferred format for comments (I see both styles throughout the codebase). FWIW it's probably worth pointing out that the style for multi-line comments isn't given in CODING_STYLE, and checkpatch didn't complain either... ATB, Mark.
On Tue, Jul 11, 2017 at 09:26:22PM +0100, Mark Cave-Ayland wrote: > On 11/07/17 21:18, Michael S. Tsirkin wrote: > > > Multi-line comments also should formatted > > > > /* > > * like > > * this > > */ > > > > not > > > > /* like > > * this */ > > > > Interesting, I never knew there was a preferred format for comments (I > see both styles throughout the codebase). > > FWIW it's probably worth pointing out that the style for multi-line > comments isn't given in CODING_STYLE, and checkpatch didn't complain > either... I just noticed that, too. I will submit a patch.
On 11 July 2017 at 21:26, Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote: > On 11/07/17 21:18, Michael S. Tsirkin wrote: > >> Multi-line comments also should formatted >> >> /* >> * like >> * this >> */ >> >> not >> >> /* like >> * this */ >> > > Interesting, I never knew there was a preferred format for comments (I > see both styles throughout the codebase). It's basically GNU coding standard style vs Linux kernel style; there's a mix because some contributors are more used to working on the kernel, and some more used to working with gcc, glibc, etc, and we haven't made a firm "comments must be like this" statement (and of course historical practice in the codebase is all over the place). We also have both of the flavours the kernel style guide documents: https://www.kernel.org/doc/html/v4.10/process/coding-style.html#commenting (I usually use the style the kernel has for net/ personally.) So I don't think that "we" the project have a preferred format; but "we" individual contributors probably have individual preferences ;-) thanks -- PMM
On Tue, Jul 11, 2017 at 09:49:30PM +0100, Peter Maydell wrote: > On 11 July 2017 at 21:26, Mark Cave-Ayland > <mark.cave-ayland@ilande.co.uk> wrote: > > On 11/07/17 21:18, Michael S. Tsirkin wrote: > > > >> Multi-line comments also should formatted > >> > >> /* > >> * like > >> * this > >> */ > >> > >> not > >> > >> /* like > >> * this */ > >> > > > > Interesting, I never knew there was a preferred format for comments (I > > see both styles throughout the codebase). > > It's basically GNU coding standard style vs Linux kernel style; > there's a mix because some contributors are more used to working > on the kernel, and some more used to working with gcc, glibc, > etc, and we haven't made a firm "comments must be like this" > statement (and of course historical practice in the codebase > is all over the place). > > We also have both of the flavours the kernel style guide > documents: > https://www.kernel.org/doc/html/v4.10/process/coding-style.html#commenting > (I usually use the style the kernel has for net/ personally.) > > So I don't think that "we" the project have a preferred > format; but "we" individual contributors probably > have individual preferences ;-) Thanks for the info. Please forget when I said I was going to send a CODING_STYLE patch for that. :)
On 11 July 2017 at 21:58, Eduardo Habkost <ehabkost@redhat.com> wrote: > On Tue, Jul 11, 2017 at 09:49:30PM +0100, Peter Maydell wrote: >> It's basically GNU coding standard style vs Linux kernel style; >> there's a mix because some contributors are more used to working >> on the kernel, and some more used to working with gcc, glibc, >> etc, and we haven't made a firm "comments must be like this" >> statement (and of course historical practice in the codebase >> is all over the place). >> >> We also have both of the flavours the kernel style guide >> documents: >> https://www.kernel.org/doc/html/v4.10/process/coding-style.html#commenting >> (I usually use the style the kernel has for net/ personally.) >> >> So I don't think that "we" the project have a preferred >> format; but "we" individual contributors probably >> have individual preferences ;-) > > Thanks for the info. Please forget when I said I was going to > send a CODING_STYLE patch for that. :) I was just giving the historical context, not necessarily arguing that we shouldn't try to impose a standard style now. (I have no strong opinion on that, beyond that style recommendations backed up by patches to checkpatch win over those without.) thanks -- PMM
(off-topic) On 07/11/17 22:49, Peter Maydell wrote: > On 11 July 2017 at 21:26, Mark Cave-Ayland > <mark.cave-ayland@ilande.co.uk> wrote: >> On 11/07/17 21:18, Michael S. Tsirkin wrote: >> >>> Multi-line comments also should formatted >>> >>> /* >>> * like >>> * this >>> */ >>> >>> not >>> >>> /* like >>> * this */ >>> >> >> Interesting, I never knew there was a preferred format for comments (I >> see both styles throughout the codebase). > > It's basically GNU coding standard style vs Linux kernel style; > there's a mix because some contributors are more used to working > on the kernel, and some more used to working with gcc, glibc, > etc, and we haven't made a firm "comments must be like this" > statement (and of course historical practice in the codebase > is all over the place). > > We also have both of the flavours the kernel style guide > documents: > https://www.kernel.org/doc/html/v4.10/process/coding-style.html#commenting > (I usually use the style the kernel has for net/ personally.) > > So I don't think that "we" the project have a preferred > format; but "we" individual contributors probably > have individual preferences ;-) Hey I'm feeling oppressed now; can I add edk2-style comments // // like this // along with CamelCaseVariables, TYPEDEFS_FOR_STRUCTS, and CamelCaseEnumConstants? ;) (Just kidding! :) I should be sleeping now. Sorry.) Laszlo
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c index 99bdbc2..8ef889a 100644 --- a/hw/nvram/fw_cfg.c +++ b/hw/nvram/fw_cfg.c @@ -1017,7 +1017,12 @@ FWCfgState *fw_cfg_init_mem(hwaddr ctl_addr, hwaddr data_addr) FWCfgState *fw_cfg_find(void) { - return FW_CFG(object_resolve_path(FW_CFG_PATH, NULL)); + /* Returns FWCfgState if only one fw_cfg device type exists. If zero or + more than one fw_cfg device are found then NULL is returned as per the + object_resolve_path_type() documentation. This behaviour is correct as + it ensures that we detect both missing fw_cfg devices and multiple + fw_cfg devices which could result in unpredictable behaviour. */ + return FW_CFG(object_resolve_path_type("", TYPE_FW_CFG, NULL)); } static void fw_cfg_class_init(ObjectClass *klass, void *data)
This will enable the fw_cfg device to be placed anywhere within the QOM tree regardless of its machine location. Note that we also add a comment to document the behaviour that we return NULL to indicate failure where either no fw_cfg device or multiple fw_cfg devices are found. Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> --- hw/nvram/fw_cfg.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)