Daniel P. Berrangé <[email protected]> writes:
> On Wed, Feb 25, 2026 at 11:35:15AM +0530, Prasad Pandit wrote:
>> On Wed, 25 Feb 2026 at 09:23, Ani Sinha <[email protected]> wrote:
>> > Currently the code that adds a migration blocker does not check if the same
>> > blocker already exists. Return an EEXIST error code if there is an attempt
>> > to
>> > add the same migration blocker again. This way the same migration blocker
>> > will
>> > not get added twice.
>> >
>> > Suggested-by: Prasad Pandit <[email protected]>
>> > Signed-off-by: Ani Sinha <[email protected]>
>> > ---
>> > migration/migration.c | 4 ++++
>> > 1 file changed, 4 insertions(+)
>> >
>> > diff --git a/migration/migration.c b/migration/migration.c
>> > index a5b0465ed3..1eb75fb7fb 100644
>> > --- a/migration/migration.c
>> > +++ b/migration/migration.c
>> > @@ -1702,6 +1702,10 @@ static int add_blockers(Error **reasonp, unsigned
>> > modes, Error **errp)
>
> This method has an '**errp' parameter.....
>
>> > {
>> > for (MigMode mode = 0; mode < MIG_MODE__MAX; mode++) {
>> > if (modes & BIT(mode)) {
>> > + if (g_slist_index(migration_blockers[mode],
>> > + *reasonp) >= 0) {
>> > + return -EEXIST;
>
> .... so using -errno for return values is not appropriate - it must
> set 'errp' and return -1.
Yes, it must set @errp on failure. Returning -1 would be wrong,
because:
int migrate_add_blocker_modes(Error **reasonp, unsigned modes, Error **errp)
{
if (is_only_migratable(reasonp, modes, errp)) {
return -EACCES;
} else if (is_busy(reasonp, errp)) {
return -EBUSY;
}
return add_blockers(reasonp, modes, errp);
}
If callers don't actually care for error codes, the entire nest of
functions could be simplified to return 0/-1 or true/false.
>> > + }
>> > migration_blockers[mode] =
>> > g_slist_prepend(migration_blockers[mode],
>> > *reasonp);
>> > }
>>
>> * Looks okay.
>> Reviewed-by: Prasad Pandit <[email protected]>
>>
>> Thank you.
>> ---
>> - Prasad
>>
>>
>
> With regards,
> Daniel
