[lttng-dev] [PATCH] lttng-tools lttng commands add_context.c :Return values of the cmd_add_context() chain of functions

[David Goulet]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1



On 12-01-27 11:36 AM, Thibault, Daniel wrote:
> -----Message d'origine-----
> De : David Goulet [mailto:dgoulet at efficios.com] 
> Envoyé : 27 janvier 2012 10:51
> 
>>> +		/*
>>> +		 * lttng_add_context() returns the size
>>> +		 * of the returned payload data
>>> +		 * or a negative error code.
>>> +		 */
>>
>> I did not had this because the API is well documented so it would duplicate
>> comments across the code and I prefer not since changing the API makes the
>> comments useless.
> 
>    As for claiming the "API is well documented", I obviously beg to differ.  :-)

I was talking about lttng.h, the documentation is, I think, good. Any
improvement is very welcome. ;)

> 
>    I put that comment in more to justify my code edits than just to "comment the code", so leaving it out of the patch is fine.
> 
>>>  		ret = lttng_add_context(handle, &context, opt_event_name,
>>>  				opt_channel_name);
>>> +		/* Stop looping upon error */
>>>  		if (ret < 0) {
>>>  			fprintf(stderr, "%s: ", type->opt->symbol);
>>> -			continue;
>>> +			goto error;
>>
>> Ok, here I am really not sure. The add-context command allows multiple "-t"
>> options and they are all added one by one and not in one single blob. So, if the
>> add context fails, you get an error on stderr and the next context is processed.
>> So, unless I missed something here, it's the correct behavior.
> 
>    The problem with that behaviour is that it is not reflected in the return code of add_context().  The original code would return CMD_SUCCESS or CMD_ERROR depending on which -t option is processed last (assuming we have a -t option that fails), something I'm very uncomfortable with.
> 
>    A possible solution would be to add a new result code, say "CMD_WARNINGS", and treat the failing lttng_add_context() calls as warnings rather than errors.  Or maybe add an option "-w|--stoponwarn" so the user can decide what the behaviour should be.
> 

Yep, I see your point now. I agree that a CMD_WARNING could be a way to go. The
command will complete but without being a success telling the user to look in
stderr for partial problem.

I'm fine with that. Yannick, do you have any suggestion about that? For me, the
warning seems the best choice.

Cheers
David

> Daniel U. Thibault
> R & D pour la défense Canada - Valcartier (RDDC Valcartier) / Defence R&D Canada - Valcartier (DRDC Valcartier)
> Système de systèmes (SdS) / System of Systems (SoS)
> Solutions informatiques et expérimentations (SIE) / Computing Solutions and Experimentations (CSE)
> 2459 Boul. Pie XI Nord
> Québec, QC  G3J 1X5
> CANADA
> Vox : (418) 844-4000 x4245
> Fax : (418) 844-4538
> NAC: 918V QSDJ
> Gouvernement du Canada / Government of Canada
> <http://www.valcartier.drdc-rddc.gc.ca/>
> 
> _______________________________________________
> lttng-dev mailing list
> lttng-dev at lists.lttng.org
> http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
> 
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)

iQEcBAEBAgAGBQJPItQsAAoJEELoaioR9I02C2oIALUsJeVWZQpHlB3kIV0jF2e2
Ncf4yS4nX/aNG4TFxl1UDJTG2eLId38gthKr8KBmAUp61MzX8eHYPvIYswStsyMq
qDV9xQjJ1Jm/Mh9vJfaDwM3Tv4qF3/HlfMEL3+iZzeE+0Q74ZpjZD53AQiJ5U+/J
A0jh9byn3cLWPlwbxy44MoHe5TOy3kTpc0h/Etbk0WldtBBwbGrAAiYaGA/Itdh1
8EFldCUegTh4Y8mSpwXYxCW+HivVWvuBFnP/u/jJL82E4wAN5AEJoBnKO/ybyAS6
foRSORdUFJ0ooHkjpmABoGKTW6KErDA3eLlI8TDKyqZryticaIYSpUwG1UFZOVs=
=e4jL
-----END PGP SIGNATURE-----