3
\$\begingroup\$

This program reads raw pixels from the pipe/stdin in a loop and set the root window background.

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <X11/Xatom.h>
#include <X11/Xlib.h>
#include <X11/Xutil.h>

int
main(int argc, char **argv)
{
    Display *dpy;
    Window root;
    XImage *ximg;
    Pixmap pm;
    GC gc;
    int scrn, fmt, ret;
    char *img;
    unsigned int w, h;
    Atom aroot, aeroot, atype;
    unsigned char *data_root, *data_eroot;
    size_t len, after, siz, nread;
    ret = EXIT_SUCCESS;
    dpy = XOpenDisplay(NULL);
    scrn = DefaultScreen(dpy);
    root = RootWindow(dpy, scrn);
    w = DisplayWidth(dpy, scrn);
    h = DisplayHeight(dpy, scrn);
    siz = 4 * w * h;
    img = malloc(siz);
    ximg = XCreateImage(dpy, CopyFromParent, 24, ZPixmap, 0, img, w, h, 32, 0);
    pm = XCreatePixmap(dpy, root, w, h, 24);
    aroot = XInternAtom(dpy, "_XROOTMAP_ID", True);
    aeroot = XInternAtom(dpy, "ESETROOT_PMAP_ID", True);
    if (aroot != None && aeroot != None) {
        XGetWindowProperty(dpy, root, aroot, 0L, 1L, False, AnyPropertyType,
                           &atype, &fmt, &len, &after, &data_root);
        if (atype == XA_PIXMAP) {
            XGetWindowProperty(dpy, root, aeroot, 0L, 1L, False, AnyPropertyType,
                               &atype, &fmt, &len, &after, &data_eroot);
            if (data_root && data_eroot && atype == XA_PIXMAP
                && *((Pixmap *) data_root) == *((Pixmap *) data_eroot))
                XKillClient(dpy, *((Pixmap *) data_root));
        }
    }
    aroot = XInternAtom(dpy, "_XROOTPMAP_ID", False);
    aeroot = XInternAtom(dpy, "ESETROOT_PMAP_ID", False);
    if (aroot == None || aeroot == None) {
        fputs("atomic disaster\n", stderr);
        ret = EXIT_FAILURE;
        goto exit;
    }
    XChangeProperty(dpy, root, aroot, XA_PIXMAP, 32, PropModeReplace,
                    (unsigned char *)&pm, 1);
    XChangeProperty(dpy, root, aeroot, XA_PIXMAP, 32, PropModeReplace,
                    (unsigned char *)&pm, 1);
    XSetCloseDownMode(dpy, RetainTemporary);
    XFlush(dpy);
    gc = XCreateGC(dpy, pm, 0, NULL);
    while ((nread = fread(img, 1, siz, stdin)) == siz) {
        XPutImage(dpy, pm, gc, ximg, 0, 0, 0, 0, w, h);
        XKillClient(dpy, AllTemporary);
        XSetWindowBackgroundPixmap(dpy, root, pm);
        XClearWindow(dpy, root);
        XFlush(dpy);
        XSync(dpy, False);
    }
    if (feof(stdin) && nread != 0) {
        fputs("bad input\n", stderr);
        ret = EXIT_FAILURE;
    } else if (ferror(stdin)) {
        perror("");
        ret = EXIT_FAILURE;
    }
exit:
    XFreeGC(dpy, gc);
    XFreePixmap(dpy, pm);
    XDestroyImage(ximg);
    XCloseDisplay(dpy);
    return ret;
}

How do I handle Xlib function errors? And should I handle all of them? Virtually every function here can return an error, even stdio ones and if I wrap each in an if statement the code would be infinite (if (fputs("msg", stderr) == EOF) { if (fputs...).

Does setting the buffer size with setvbuf make sense since I know upfront that the input size should be the multiple of 4 * w * h which is in the order of megabytes? As per the second answer to similar question improvement would be minimal, but I'm not sure how to verify it.

I am using it with stream from ImageMagick, like this:

stream -map bgra  wallpaper.png - | myprog

bgra instead of rgba because I couldn't find any proper way around this and unlike the OP I had simpler solution than the slow loop. And a because if I did just rgb bottom 1/4th of the screen looked like it was broken, even though the depth in XCreateImage is set to 24. So only png format works for now, since stream freaks out when I tell it to add alpha to jpg and co.

\$\endgroup\$
0

1 Answer 1

Reset to default
2
\$\begingroup\$

Complexity

The function main() is too complex (does too much). As programs grow in size the use of main() should be limited to calling functions that parse the command line, calling functions that set up for processing, calling functions that execute the desired function of the program, and calling functions to clean up after the main portion of the program.

There is also a programming principle called the Single Responsibility Principle that applies here. The Single Responsibility Principle states:

that every module, class, or function should have responsibility over a single part of the functionality provided by the software, and that responsibility should be entirely encapsulated by that module, class or function.

You can add a function to provide error handling and pass the result of each call that can fail to the error handling function. The error handling function can either return a status or call the exit(status) function after reporting the error.

Test for Possible Memory Allocation Errors

In modern high level languages such as C++, memory allocation errors throw an exception that the programmer can catch. This is not the case in the C programming language. While it rare in modern computers because there is so much memory, memory allocation can fail, especially if the code is working in a limited memory application such as embedded control systems. In the C programming language when memory allocation fails, the functions malloc(), calloc() and realloc() return NULL. Referencing any memory address through a NULL pointer results in undefined behavior (UB).

Possible behavior in this case can be a memory page error (in Unix this would be call Segmentation Violation), corrupted data in the program and in very old computers it could even cause the computer to reboot (corruption of the stack pointer).

To prevent this undefined behavior a best practice is to always follow the memory allocation statement with a test that the pointer that was returned is not NULL.

Current Code

    siz = 4 * w * h;
    img = malloc(siz);
    ximg = XCreateImage(dpy, CopyFromParent, 24, ZPixmap, 0, img, w, h, 32, 0);

Example of Current Code with Test:

    unsigned int siz = 4 * w * h;
    char *img = malloc(siz);
    if (img == NULL)
    {
        fprintf(stderr, "Malloc of siz failed\n");
        exit(EXIT_FAILURE);
    }

    ximg = XCreateImage(dpy, CopyFromParent, 24, ZPixmap, 0, img, w, h, 32, 0);

Magic Numbers

The previous code snippet contains two magic numbers (4 and 24), it might be better to create symbolic constants for them to make the code more readable and easier to maintain. These numbers may be used in many places and being able to change them by editing only one line makes maintenance easier.

Numeric constants in code are sometimes referred to as Magic Numbers, because there is no obvious meaning for them. There is a discussion of this on stackoverflow.

Declaring and Initializing Variables

The C programming language is not friendly and does not provide a default initialization value when a variable is declared. A variable should always be initialized when it is declared. To make it easier to maintain the code each variable should be declared and initialized in its own statement.

As the suggested code for handling malloc() declare the variables as they are needed rather than putting all the variable declarations at the top of the function. The original version of C required all variables to be declared at the top of the scope, but that hasn't been true for over 30 years.

Current Code

    int scrn, fmt, ret;
    char *img;
    unsigned int w, h;

Suggested Code

    int scrn = 0;
    int fmt = 0;
    int program status = EXIT_SUCCESS;
    char *img = NULL;
    unsigned int width = 0; 
    unsigned int height = 0;
\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.