From mboxrd@z Thu Jan  1 00:00:00 1970
From: Dario Faggioli <dario.faggioli@citrix.com>
Subject: Re: [PATCH 14/24] libxl: allow to set the ratelimit
 value online for Credit2
Date: Fri, 30 Sep 2016 03:03:06 +0200
Message-ID: <1475197386.5315.55.camel@citrix.com>
References: <147145358844.25877.7490417583264534196.stgit@Solace.fritz.box>
 <147145435166.25877.2550918854619355050.stgit@Solace.fritz.box>
 <22458.50627.655327.641414@mariner.uk.xensource.com>
Mime-Version: 1.0
Content-Type: multipart/mixed; boundary="===============2354475024135880337=="
Return-path: <xen-devel-bounces@lists.xen.org>
Received: from mail6.bemta6.messagelabs.com ([193.109.254.103])
 by lists.xenproject.org with esmtp (Exim 4.84_2)
 (envelope-from <prvs=074d4ead7=dario.faggioli@citrix.com>)
 id 1bpmER-0002mT-QY
 for xen-devel@lists.xenproject.org; Fri, 30 Sep 2016 01:03:19 +0000
In-Reply-To: <22458.50627.655327.641414@mariner.uk.xensource.com>
List-Unsubscribe: <https://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
 <mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <https://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
 <mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Errors-To: xen-devel-bounces@lists.xen.org
Sender: "Xen-devel" <xen-devel-bounces@lists.xen.org>
To: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: George Dunlap <george.dunlap@eu.citrix.com>, xen-devel@lists.xenproject.org, Wei Liu <wei.liu2@citrix.com>
List-Id: xen-devel@lists.xenproject.org

--===============2354475024135880337==
Content-Type: multipart/signed; micalg=pgp-sha256;
	protocol="application/pgp-signature"; boundary="=-tS9aN4IGEvGUdbW/Ofej"

--=-tS9aN4IGEvGUdbW/Ofej
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Mon, 2016-08-22 at 10:28 +0100, Ian Jackson wrote:
> Dario Faggioli writes ("[PATCH 14/24] libxl: allow to set the
> ratelimit value online for Credit2"):
> ...
> >=20
> > -=C2=A0=C2=A0=C2=A0=C2=A0rc =3D xc_sched_credit_params_set(ctx->xch, po=
olid, &sparam);
> > -=C2=A0=C2=A0=C2=A0=C2=A0if ( rc < 0 ) {
> > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0LOGE(ERROR, "setting s=
ched credit param");
> > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0GC_FREE;
> > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0return ERROR_FAIL;
> > +=C2=A0=C2=A0=C2=A0=C2=A0r =3D xc_sched_credit_params_set(ctx->xch, poo=
lid, &sparam);
> > +=C2=A0=C2=A0=C2=A0=C2=A0if ( r < 0 ) {
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0LOGE(ERROR, "Setting C=
redit scheduler parameters");
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0rc =3D ERROR_FAIL;
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0goto out;
>=20
> I had to read this three times to figure out what the change was.
>=20
> It is good that you are fixing the coding style but can you please
> put
> it in a separate patch ?
>=20
Done in v2.

> But I wonder whether there will still be lots of rather formulaic
> code
> that could profitably be generalised somehow.=C2=A0=C2=A0I'd appreciate y=
our
> views on whether that would be possible, and whether it would be a
> good idea..
>=20
I've checked, as promised. TBH, as George said already, I don't see
much more room for factoring or generalizing. Certainly, not in libxl.

In xl (that would be next patch, though), I especially dislike
those=C2=A0main_sched_credit(),=C2=A0main_sched_credit2(), etc., but I thin=
k
that, given how the interface looks like, there is again few that we
can do (there's some level of generalization and indirection already,
actually).

So, again, apart from splitting coding style and functional changes, I
don't see other ways for improving this patch. If you have more
concrete ides, please share them. :-)

Thanks and Regards,
Dario
--=20
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
--=-tS9aN4IGEvGUdbW/Ofej
Content-Type: application/pgp-signature; name="signature.asc"
Content-Description: This is a digitally signed message part
Content-Transfer-Encoding: 7bit

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAABCAAGBQJX7bnKAAoJEBZCeImluHPuZbMP/A9m2iUVReLk0sGVyWX5r2ke
nIeh+jNQzeXz1sWMbu9htPKShEoSoIW9BaaO6WFoDwdgswsvsBbt+ATtVuIhsY1z
Ml+3fknKO0Aq9fwHYmsJWwcA2DRVAPtlDsfMqlJlGvuwzoqQFOrt7A66ALFlfchp
7o2MKi7X+JiAXFxpAacv/Nb7yx5t1AohXGy8YLZE9RlW6ignSGdC1gWHe9+/NbF6
CJj5E25Wziqp8v6Ic8bbO9hp2QfFDDTJ3hyEC/1l0WIlHNTl6Kq3PW2CKUq9+DqN
b99C+szHfmGhv8grU/qswlgAyecXYp4dA+OWd8xbWrEu6OJBuNxiSbegCc5vbToj
pPwPJlTsFzLwPO9gTblA9oQM3Ru8WkDBd61KWY8EumY8xEgtyHgFRpqUc8/B8Z1R
Un+JWcpDn7QZE8Df1sz2QItNlLv/jkCIom2wnBE6O1a4EV2Jv0OjaocOXikJKPp6
s4zMtj1QbRHbtMN0DTcHejx4Vw9vGOci//lA6KoIOcMNA5ezxhGQy7nuYkVsEyuX
J/4GTQw+7gNNAFPVWgPFVfV2lEBTQ/+dMKhgcemCYaHruI5pNubTrL5kLXwTT+Qp
bwMnfhylug36oLWNPRkT2FfnCl/Rh7+7Jz5FiHsxYFC9NwxOHbALWIu3Trg8cNlQ
Y6TFhZzbSxtPfyvRTp29
=00xw
-----END PGP SIGNATURE-----

--=-tS9aN4IGEvGUdbW/Ofej--


--===============2354475024135880337==
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: base64
Content-Disposition: inline

X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs
IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwczovL2xpc3RzLnhlbi5v
cmcveGVuLWRldmVsCg==

--===============2354475024135880337==--