From: Andreas Oberritter <obi@linuxtv.org>
To: Mauro Carvalho Chehab <mchehab@redhat.com>
Cc: linux-media@vger.kernel.org, Thierry LELEGARD <tlelegard@logiways.com>
Subject: Re: [PATCH 2/8] DVB: dtv_property_cache_submit shouldn't modifiy the cache
Date: Mon, 09 May 2011 12:12:07 +0200 [thread overview]
Message-ID: <4DC7BDF7.1020706@linuxtv.org> (raw)
In-Reply-To: <4DC769A6.1090100@redhat.com>
On 05/09/2011 06:12 AM, Mauro Carvalho Chehab wrote:
> Em 09-05-2011 05:58, Mauro Carvalho Chehab escreveu:
>> Em 09-05-2011 01:03, Andreas Oberritter escreveu:
>>> - Use const pointers and remove assignments.
>>
>> That's OK.
>>
>>> - delivery_system already gets assigned by DTV_DELIVERY_SYSTEM
>>> and dtv_property_cache_sync.
>>
>> The logic for those legacy params is too complex. It is easy that
>> a latter patch to break the implicit set via dtv_property_cache_sync().
>>
>> Do you actually see a bug caused by the extra set for the delivery system?
>> If not, I prefer to keep this extra re-assignment.
No, but I was suprised to see the dtv_frontend_properties getting
modified in this function. There are three functions converting between
old and new structures:
dtv_property_cache_sync:
converts from dvb_frontend_parameters to dtv_frontend_properties
dtv_property_legacy_params_sync and dtv_property_adv_params_sync:
convert from dtv_frontend_properties to dvb_frontend_parameters
Assigning to fields of a source structure indicates a logical error. I
haven't found any comment on why this should be needed in the Git
history or in the mailing list archives.
Btw., I think that two functions should be enough. Any reason for not
merging dtv_property_adv_params_sync into
dtv_property_legacy_params_sync and calling the latter unconditionally?
> Hmm... after applying all patches the logic change, and patch 2 may actually
> make sense. I'll need to re-examine the patch series.
>
> On a quick look, if applied as-is, I suspect that git bisect
> will break dvb in the middle of the patch series.
Patches 6 and 8 depend on the patches mentioned in the cover letter. All
other patches should apply and compile cleanly. Which patch do you
suspect to break bisectability?
> Anyway, patch 1/8 is OK. For now, I'll apply only this patch. I'll delay the
> others until I have more time. I'm currently traveling abroad, due to Linaro
> Development Summit, so, I don't have much time for review (and I'm also suffering
> for a 5 hours jet-leg).
OK, there's no hurry.
Regards,
Andreas
next prev parent reply other threads:[~2011-05-09 10:12 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-05-08 23:03 [PATCH 0/8] dvb_frontend cleanup, S2API regression fix Andreas Oberritter
2011-05-08 23:03 ` [PATCH 1/8] DVB: return meaningful error codes in dvb_frontend Andreas Oberritter
2011-05-08 23:03 ` [PATCH 2/8] DVB: dtv_property_cache_submit shouldn't modifiy the cache Andreas Oberritter
2011-05-09 3:58 ` Mauro Carvalho Chehab
2011-05-09 4:12 ` Mauro Carvalho Chehab
2011-05-09 10:12 ` Andreas Oberritter [this message]
2011-05-08 23:03 ` [PATCH 3/8] DVB: call get_property at the end of dtv_property_process_get Andreas Oberritter
2011-05-08 23:03 ` [PATCH 4/8] DVB: dvb_frontend: rename parameters to parameters_in Andreas Oberritter
2011-05-08 23:03 ` [PATCH 5/8] DVB: dvb_frontend: remove unused assignments Andreas Oberritter
2011-05-08 23:03 ` [PATCH 6/8] DVB: dvb_frontend: use shortcut to access fe->dtv_property_cache Andreas Oberritter
2011-05-08 23:03 ` [PATCH 7/8] DVB: dvb_frontend: add parameters_out Andreas Oberritter
2011-05-08 23:03 ` [PATCH 8/8] DVB: allow to read back of detected parameters through S2API Andreas Oberritter
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4DC7BDF7.1020706@linuxtv.org \
--to=obi@linuxtv.org \
--cc=linux-media@vger.kernel.org \
--cc=mchehab@redhat.com \
--cc=tlelegard@logiways.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox