Re: New command to check NT's hibernation state

[Vladimir 'φ-coder/phcoder' Serbinenko]

On 03.12.2011 05:57, Peter Lustig wrote:
> /* nthibr.c - tests whether an MS Windows system partition is hibernated */
> /*
>   *  GRUB  --  GRand Unified Bootloader
>   *  Copyright (C) 2007,2008,2009,2011  Free Software Foundation, Inc.
>   *
>   *  GRUB is free software: you can redistribute it and/or modify
>   *  it under the terms of the GNU General Public License as published by
>   *  the Free Software Foundation, either version 3 of the License, or
>   *  (at your option) any later version.
>   *
>   *  GRUB is distributed in the hope that it will be useful,
>   *  but WITHOUT ANY WARRANTY; without even the implied warranty of
>   *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>   *  GNU General Public License for more details.
>   *
>   *  You should have received a copy of the GNU General Public License
>   *  along with GRUB.  If not, see<http://www.gnu.org/licenses/>.
>   */
>
> #include<grub/types.h>
> #include<grub/mm.h>
> #include<grub/disk.h>
> #include<grub/file.h>
> #include<grub/misc.h>
> #include<grub/dl.h>
> #include<grub/extcmd.h>
> #include<grub/lib/arg.h>
> #include<grub/err.h>
> #include<grub/i18n.h>
>
> GRUB_MOD_LICENSE("GPLv3+");
>
> /* Define this 'empty' array to let the '-h' and '-u' switches be processed */
> static const struct grub_arg_option options[] = {
>    {0, 0, 0, 0, 0, 0}
> };
No need for empty array. passing 0 instead of this array works.
> static grub_err_t
> grub_cmd_nthibr (grub_extcmd_context_t ctxt __attribute__ ((unused)),
>                   int argc, char **args)
> {
>    grub_err_t status = GRUB_ERR_NONE;
>    char *partition_name, *hibr_file_path = 0, hibr_file_magic[5];
>    grub_size_t name_length;
>    grub_ssize_t magic_size;
>    grub_disk_t partition;
>    grub_file_t hibr_file = 0;
>
>    /* Check argument count */
>    if (!argc)
>      {
>        status = grub_error (GRUB_ERR_BAD_ARGUMENT,
>                             N_("too few arguments specified"));
>        goto exit;
>      }
>    else if (argc>  1)
>      {
>        status = grub_error (GRUB_ERR_BAD_ARGUMENT,
>                             N_("too many arguments specified"));
>        goto exit;
>      }
>
>    partition_name = args[0];
>    name_length = grub_strlen (partition_name);
>
>    /* Check if partition specifier 'looks right' */
>    if (partition_name[0] != '(' || partition_name[name_length - 1] != ')')
>      {
>        status = grub_error (GRUB_ERR_BAD_FILENAME,
>                             N_("invalid partition specifier"));
>        goto exit;
>      }
I would recommend adding '(' and ')' if none found rather than giving error.
>    /* Check if partition actually exists */
>    partition_name[name_length - 1] = '\0';
>    partition = grub_disk_open (partition_name + 1);
>    if (!partition)
>      {
>        status = grub_error (GRUB_ERR_UNKNOWN_DEVICE,
>                             N_("partition not found"));
No need to run grub_error here. grub_disk_open already sets grub_errno 
and grub_errmsg. Just status = grub_errno is enough.
>        goto exit;
>      }
>    else
>      {
>        grub_disk_close (partition);
>        partition_name[name_length - 1] = ')';
>      }
No need for 'else' clause (goto already takes care of it)
>    /* Build path to 'hiberfil.sys' */
>    hibr_file_path = grub_malloc (grub_strlen (partition_name)
>                                  + grub_strlen ("/hiberfil.sys") + 1);
>    if (!hibr_file_path)
>      {
>        status = grub_error (GRUB_ERR_OUT_OF_MEMORY,
>                             N_("out of memory"));
>        goto exit;
>      }
>    else
>      {
>        grub_strcpy (hibr_file_path, partition_name);
>        grub_strcat (hibr_file_path, "/hiberfil.sys");
>      }
Same here (no need for grub_error and else)
>    /* Try to open 'hiberfil.sys' */
>    hibr_file = grub_file_open (hibr_file_path);
>    if (!hibr_file)
>      {
>        status = grub_error (GRUB_ERR_FILE_NOT_FOUND,
>                             N_("'hiberfil.sys' not found"));
ditto
>        goto exit;
>      }
>
>    /* Try to read magic number of 'hiberfil.sys' */
>    magic_size = sizeof (hibr_file_magic) - 1;
>    grub_memset (hibr_file_magic, 0, sizeof (hibr_file_magic));
>    if (grub_file_read (hibr_file, hibr_file_magic, magic_size)<  magic_size)
>      {
>        status = grub_error (GRUB_ERR_BAD_FILE_TYPE,
>                             N_("'hiberfil.sys' too small"));
you need to conditionalize call to grub_error. if (!grub_errno) ... 
since incomplete read may indicate an FS problem in which case you want 
the original error message to remain.
>        goto exit;
>      }
>
>    /* Return SUCCESS if magic indicates file is active; else return FAILURE */
>    if (!grub_strncasecmp ("hibr", hibr_file_magic, magic_size))
>      grub_puts (N_("The system is hibernated."));
>    else
>      {
>        grub_puts (N_("The system is NOT hibernated."));
>        status = GRUB_ERR_TEST_FAILURE;
>      }
We also do grub_error (GRUB_ERR_TEST_FAILURE, "false");
> exit:
>    /* Ensure unmanaged resources are cleaned up */
>    if (hibr_file_path)
>      grub_free (hibr_file_path);
grub_free already checks for not-NULL pointers.
>    if (hibr_file)
>      grub_file_close (hibr_file);
>
>    return status;
You can eliminate status and the need of keeping one by using return 
grub_errno;
> }


--
Regards
Vladimir 'φ-coder/phcoder' Serbinenko