After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 583778 - change script-fu-server to return the output of executed commands
change script-fu-server to return the output of executed commands
Status: RESOLVED FIXED
Product: GIMP
Classification: Other
Component: Script-Fu
git master
Other All
: Normal enhancement
: 2.10
Assigned To: GIMP Bugs
GIMP Bugs
Depends on:
Blocks:
 
 
Reported: 2009-05-25 07:54 UTC by Ionutz Borcoman
Modified: 2012-12-14 18:19 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
changes script-fu-server to return the output of the executed commands (754 bytes, patch)
2009-05-25 07:55 UTC, Ionutz Borcoman
reviewed Details | Review

Description Ionutz Borcoman 2009-05-25 07:54:31 UTC
Currently, script-fu-server will invariably return 'Success' on executing successfully a command. This is of little use, especially considering that we already have the error byte for this.

A much better behavior would be to actually return the output of the executed commands and leave the error byte to indicate if the commands were successful or not.

Other information:
Comment 1 Ionutz Borcoman 2009-05-25 07:55:37 UTC
Created attachment 135300 [details] [review]
changes script-fu-server to return the output of the executed commands
Comment 2 Sven Neumann 2009-05-26 20:39:41 UTC
Let's look at this for 2.8.
Comment 3 Martin Nordholts 2010-02-17 06:20:19 UTC
Ionutz, can you give some sample code/script that can be used to test the patch? Do you see any problems with this patch? Is there a risk it will break already existing script-fu-server clients?
Comment 4 Ionutz Borcoman 2010-02-17 07:50:37 UTC
Unfortunately, I can't guarantee that. I've barely managed to poke around and find something that worked for me and, apparently, didn't break something else. That patch is my one and only gimp code I've ever done.
Comment 5 Martin Nordholts 2010-03-07 13:29:55 UTC
Ok I see. To do this change that would have to be looked into though. I'm putting this bug off the 2.8 milestone for now.
Comment 6 Tobias Mueller 2010-04-23 08:21:41 UTC
Reopening as I can't see any open question.
Comment 7 Martin Nordholts 2010-05-16 09:27:57 UTC
Review of attachment 135300 [details] [review]:

I don't see anything inherently wrong with the patch, someone just needs to test it thoroughly and think over the consequences.
Comment 8 Cyril Brulebois 2012-12-11 03:32:59 UTC
I'd like to support Ionutz's suggestion.

For some context, I'm writing a tiny Perl abstraction layer to automate sending jobs to a Gimp script-fu server. After a quick look into PDB, it looked like the following functions were perfect to make sure a connection to the server was properly opened:
  gimp-version
  gimp-image-list
  gimp-getpid

One could imagine avoiding this or that deprecated method based on (gimp-version)'s output, picking the right image thanks to (gimp-image-list), etc.

This works properly in the console, but not through a socket to the server, which isn't too nice.


Implementing Ionutz's suggestion is indeed a behaviour change, and some people might rely on the server's returning “Success” all the time. Nevertheless, it looks to me like the current behaviour is kind of broken, and that those people really ought to have reported that strange behaviour. Switching from checking for “Success” as payload to checking the error code some bytes before is trivial anyway.


I should be playing with that patch on top of 2.8.2 over the next few days, and I'll report back.
Comment 9 Cyril Brulebois 2012-12-11 04:25:52 UTC
After a few basic tests, it looks like the patch is implementing what I would have expected; thanks!

Now, what could it break? Looking at execute_command() in script-fu-server.c, and with no specific knowledge about the scheme interpreter side, one can expect the following assertions were true until now:
 - “Success” fits into a 64k string.
 - Any error message would fit into a 64k string (even though the client might craft some lengthy messages, I guess).


Now, if the user would construct say a for loop returning too long a message, that wouldn't be caught by the patched code. AFAICT, response is a (GString *) with no specific limit on its length. Returning that to a client would mean announcing a <64k size (through the high/low bytes), then sending more than that (iterating until response->len is reached in the second for loop), breaking the receiving end's expectations.

One could think of truncating the output without any warnings, which doesn't look too nice; or returning an error code along the “sorry, the command went OK, but its output is too long to return to you” line, which is still not nice, but a little more honest. There might be better solutions, but it's a few hours past sleepy times here…


Martin: http://docs.gimp.org/2.8/en/gimp-filters-script-fu.html says servertest.py is here for that purpose.

The server can be started this way:
  gimp --verbose -i -b '(plug-in-script-fu-server RUN-NONINTERACTIVE 10008 "mylogfile.txt")'

And the client that way:
  python ./plug-ins/script-fu/servertest.py localhost 10008


Before:
-------
Trying 127.0.0.1.
Script-Fu-Remote - Testclient
> (gimp-version)
 (OK): Success
> (gimp-image-list)
 (OK): Success
> 

After:
------
Trying 127.0.0.1.
Script-Fu-Remote - Testclient
> (gimp-version)
 (OK): ("2.8.2")
> (gimp-image-list)
 (OK): (0 #())
> 

Hope this helps.

Mraw,
KiBi.
Comment 10 Michael Natterer 2012-12-14 18:19:32 UTC
Thanks, fixed in master:

commit 24ff958580bb286001b0e94605a1bc0a116e8478
Author: Ionutz Borcoman <iborco@gmail.com>
Date:   Fri Dec 14 19:06:49 2012 +0100

    Bug 583778 - change script-fu-server to return the output of executed...
    
    Return the output of the command for the client to parse instead of
    always a useless "Success".

 plug-ins/script-fu/script-fu-server.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)