Re: Refactor to make use of a common function for GetSubscriptionRelations and GetSubscriptionNotReadyRelations.

On Thu, Jul 14, 2022 at 4:34 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> On Wed, Jul 13, 2022 at 7:55 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> >
> > On Wed, Jul 13, 2022 at 1:13 PM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
> > >
> > > On Wed, Jul 13, 2022 at 12:22:06PM +0530, vignesh C wrote:
> > > > Most of the code is common between GetSubscriptionRelations and
> > > > GetSubscriptionNotReadyRelations. Added a parameter to
> > > > GetSubscriptionRelations which could provide the same functionality as
> > > > the existing GetSubscriptionRelations and
> > > > GetSubscriptionNotReadyRelations. Attached patch has the changes for
> > > > the same. Thoughts?
> > >
> > > Right. Using all_rels to mean that we'd filter relations that are not
> > > ready is a bit confusing, though. Perhaps this could use a bitmask as
> > > argument.
> >
> > The attached v2 patch has the modified version which includes the
> > changes to make the argument as bitmask.
> >
>
> By using a bitmask I think there is an implication that the flags can
> be combined...
>
> Perhaps it is not a problem today, but later you may want more flags. e.g.
> #define SUBSCRIPTION_REL_STATE_READY 0x02 /* READY relations */
>
> then the bitmask idea falls apart because IIUC you have no intentions
> to permit things like:
> (SUBSCRIPTION_REL_STATE_NOT_READY | SUBSCRIPTION_REL_STATE_READY)
>
> IMO using an enum might be a better choice for that parameter.

Changed it to enum so that it can be extended to support other
subscription relations like ready state subscription relations later
easily. The attached v3 patch has the changes for the same.

Regards,
Vignesh